SelectionDAG store merging default implementations are unreasonable

[arsenm]

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.

llvm-project/llvm/include/llvm/CodeGen/TargetLowering.h

Line 681 in 9bebf25

virtual bool canMergeStoresTo(unsigned AS, EVT MemVT,

llvm-project/llvm/include/llvm/CodeGen/TargetLowering.h

Line 676 in 9bebf25

virtual bool mergeStoresAfterLegalization(EVT MemVT) const {

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

[llvmbot]

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1. Check that no other contributor has already been assigned to this issue. If you believe that no one is actually working on it despite an assignment, ping the person. After one week without a response, the assignee may be changed.
2. In the comments of this issue, request for it to be assigned to you, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
3. Fix the issue locally.
4. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
5. Create a Git commit.
6. Run git clang-format HEAD~1 to format your changes.
7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

[llvmbot]

@llvm/issue-subscribers-good-first-issue

Author: Matt Arsenault (arsenm)

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.

llvm-project/llvm/include/llvm/CodeGen/TargetLowering.h

Line 681 in 9bebf25

virtual bool canMergeStoresTo(unsigned AS, EVT MemVT,

llvm-project/llvm/include/llvm/CodeGen/TargetLowering.h

Line 676 in 9bebf25

virtual bool mergeStoresAfterLegalization(EVT MemVT) const {

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.

[zxc12523]

Hi, I would like to take this issue!

[arsenm]

I'm not sure the starter patch is interpreting MemVT correctly, I implemented it assuming it was seeing the full combined size but it's probably looking at one post-legalized element. The implementation makes more sense in canMergeStoresTo

[Devesh-Yadav10]

Hi, I would like to take this problem

[dev-the-desai]

Hey there, was wondering if someone is still working? I would like to take this issue if not! @arsenm

[TRIGON01]

Hi @arsenm, I don't see any activity here, can I have a go at it ?