llvm/llvm-project

SelectionDAG store merging default implementations are unreasonable

Open

#90.714 aperta il 1 mag 2024

Vedi su GitHub
 (12 commenti) (0 reazioni) (2 assegnatari)C++ (10.782 fork)batch import
good first issuellvm:SelectionDAG

Metriche repository

Star
 (26.378 star)
Metriche merge PR
 (Merge medio 1g 2h) (1000 PR mergiate in 30 g)

Descrizione

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

Guida contributor