llvm/llvm-project

SelectionDAG store merging default implementations are unreasonable

Open

#90.714 geöffnet am 1. Mai 2024

Auf GitHub ansehen
 (12 Kommentare) (0 Reaktionen) (2 zugewiesene Personen)C++ (10.782 Forks)batch import
good first issuellvm:SelectionDAG

Repository-Metriken

Stars
 (26.378 Stars)
PR-Merge-Metriken
 (Durchschn. Merge 1T 2h) (1.000 gemergte PRs in 30 T)

Beschreibung

The API for controlling store merging in SelectionDAG is bad, and the default implementations will infinite loop on any custom store legalization that decompose stores. This adds more work (and lost time debugging) for target maintainers, and should try to default to something safe.

https://github.com/llvm/llvm-project/blob/9bebf25ecbe6a8720dd581bd2a4f8d29aa763a42/llvm/include/llvm/CodeGen/TargetLowering.h#L681

https://github.com/llvm/llvm-project/blob/9bebf25ecbe6a8720dd581bd2a4f8d29aa763a42/llvm/include/llvm/CodeGen/TargetLowering.h#L676

If a backend custom lowers a store and decomposes it, the default implementation reporting it's just safe to merge will infinite loop the legalizer. At a minimum, one of these hooks should default to checking isOperationLegal(ISD::STORE, MemVT) instead of an unconditional true. This would be more conservatively correct.

Additionally, the API here is too simple. Especially after legalization, the register value type and the in-memory value types may be relevant for truncating stores, so these callbacks should probably have more parameters. This is another instance where SelectionDAG's expressiveness for memory operation legality is inadequate.

I tried switching the default to the sample isOperationLegal example above, and a manageable number of test cases break in various targets. To resolve this issue, those failures need to be investigated to see what useful cases were missed. If we can extend the default logic to handle them as they are, that would be ideal but backend specific implementations of these hooks may need to be updated.

Starter patch: 0001-DAG-Fix-unreasonable-default-for-canMergeStoresTo.patch

As of today, these tests fail with it applied: LLVM :: CodeGen/ARM/fp16-promote.ll LLVM :: CodeGen/BPF/undef.ll LLVM :: CodeGen/Mips/cconv/illegal-vectors.ll LLVM :: CodeGen/Mips/cconv/vector.ll LLVM :: CodeGen/Mips/llvm-ir/extractelement.ll LLVM :: CodeGen/PowerPC/mma-acc-memops.ll LLVM :: CodeGen/WebAssembly/simd-arith.ll LLVM :: CodeGen/WebAssembly/simd.ll

Contributor Guide