[DAGCombiner] Avoid combining adjacent stores at -O0 to improve debug experience

[xgupta]

Summary

When the source has a series of assignments, users reasonably want to
have the debugger step through each one individually. Turn off the combine
for adjacent stores so we get this behavior at -O0.

Similar to D7181.

[xgupta]

minor fix

[jrtc27]

llvm/test/CodeGen/RISCV/dbg-combine.ll

1 ↗

(On Diff #394592)

This is way too complicated a test case. All you need is IR that makes the same store twice and verify the codegen has the two stores in it.

It's also clearly *not* currently using utils/update_llc_test_checks.py.

[xgupta]

llvm/test/CodeGen/RISCV/dbg-combine.ll

1 ↗

(On Diff #394592)

Yes, I have tried, but check lines are not generated. I will look tomorrow at how to make that script work and think of reducing test case.

[xgupta]

Address comments

[xgupta]

There is some problem with the script when I generate .ll file with -g flag and use it to generate CHECK lines it will not work, but it work when the test case has no debug info. This debuginfo test also doesn't use the script for check lines https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/RISCV/dwarf-eh.ll.

Before patch
Address Line Column File ISA Discriminator Flags

---

0x0000000000401110 2 0 1 0 0 is_stmt
0x000000000040111b 5 8 1 0 0 is_stmt prologue_end
0x0000000000401124 9 3 1 0 0 is_stmt
0x0000000000401126 9 3 1 0 0 is_stmt end_sequence

After patch
Address Line Column File ISA Discriminator Flags

---

0x0000000000010180 2 0 1 0 0 is_stmt
0x0000000000010190 5 8 1 0 0 is_stmt prologue_end
0x0000000000010194 6 8 1 0 0 is_stmt
0x0000000000010198 7 8 1 0 0 is_stmt
0x000000000001019c 9 3 1 0 0 is_stmt
0x00000000000101a4 9 3 1 0 0 is_stmt end_sequence

[jrtc27]

llvm/test/CodeGen/RISCV/dbg-combine.ll

1 ↗

(On Diff #394592)

You don't need to look at the debug info. The only thing you need to check is that the assembly has the redundant store in it. That's what this patch is actually changing, so is what should be tested.

[xgupta]

.

[xgupta]

llvm/test/CodeGen/RISCV/dbg-combine.ll

1 ↗

(On Diff #394592)

But then the actual motive (missing debuginfo) of the patch is not clear. but you are saying so I am doing that.

[jrtc27]

llvm/test/CodeGen/RISCV/dbg-combine.ll

1 ↗

(On Diff #395230)

dbg-combine isn't a great name for this simplified test, maybe something like optnone-store-no-combine?

4–6 ↗

(On Diff #395230)

(a) It's _correct_ to optimise out redundant stores, just unhelpful in your specific use case, so don't use that term here

(b) Specifying the exact instruction is a bad idea, you don't care what instruction it is and it could change in future (e.g. frame layout gets shifted about for some reason, or we change to using sp-relative rather than s0-relative), just that each store in the IR gets a corresponding store in the generated code rather than being optimised out

8–18 ↗

(On Diff #395230)

We don't tend to include C for IR tests, because the IR is supposed to have been distilled down to something simple and readable that can be understood on its own. Especially for a test as simple as this the C is a complete waste of space (and overly complicated with the use of sizeof, how the value came to be is totally irrelevant).

20 ↗

(On Diff #395230)

Return type is unnecessary, just use void

36 ↗

(On Diff #395230)

Don't think this is needed

37 ↗

(On Diff #395230)

Generally load/store tests will take a pointer as an argument to the function rather than complicating things with an alloca (i.e. @foo(i32* %p))

44 ↗

(On Diff #395230)

Way too complicated, all you care about is optnone (and _possibly_ nounwind depending on whether excluding it ends up cluttering the assembly with CFI directives), and those should both be specified inline rather than indirectly via #0

45 ↗

(On Diff #395230)

Trailing blank lines should go

[xgupta]

address comments

[xgupta]

Thank you suggestions. I get to know many things.

llvm/test/CodeGen/RISCV/dbg-combine.ll

37 ↗

(On Diff #395230)

Sorry, I am not sure how that test will look like. But if the test case should not have alloca, I make it to disappear. If the current test is not good please share an example c code.

44 ↗

(On Diff #395230)

Adding noinline also because without that -
Exit Code: 2
Command Output (stderr):
Attribute 'optnone' requires 'noinline'!

[xgupta]

comment fix

[jrtc27]

llvm/test/CodeGen/RISCV/optnone-store-no-combine.ll

6

"the those"

8–23

Like I said, it's @foo(i32* %p), then you just use %p everywhere.

I imagine it'll give the following:

; CHECK-LABEL: foo:
; CHECK:       # %bb.0:
; CHECK-NEXT:    li a1, 8
; CHECK-NEXT:    sw a1, 0(a0)
; CHECK-NEXT:    sw a1, 0(a0)
; CHECK-NEXT:    ret

I've also deleted one of the stores, you don't need three, you only need two to show that they don't get turned into a single one

[xgupta]

update

[xgupta]

Can someone accept the patch so I commit the change?

[xgupta]

I will commit as it seems not a big change. and all comments are addressed.

[jrtc27]

In D115808#3201773, @xgupta wrote:

I will commit as it seems not a big change. and all comments are addressed.

This goes against the LLVM Code-Review Policy and Practices:

Code review can be an iterative process, which continues until the patch is ready to be committed. Specifically, once a patch is sent out for review, it needs an explicit approval before it is committed. Do not assume silent approval, or solicit objections to a patch with a deadline.

• https://llvm.org/docs/CodeReview.html#code-review-workflow

I only commented on the test case to make sure you didn't make a mess of llvm/test/CodeGen/RISCV. I don't agree with the DAGCombiner change itself, which may not surprise you, as it's just trying to patch over a fundamental issue (or non-issue) with SelectionDAG's current implementation, so my lack of commenting on that was not approval, just "I don't feel strongly enough to block it, and will let someone else decide on whether it's a worthwhile change".

[xgupta]

Honestly, I am not aware that you are only reviewing the RISCV test case. I have no objection to reverting it immediately if any SelectionDAG developer objects to change.

[spatel]

I agree that it would be better to have a higher-level fix for the problem and made a similar comment here:
D111596

But it doesn't seem like there's enough motivation to overcome whatever the underlying bugs are, so we have a bunch of these 'optnone' checks around DAGCombiner now.

For reference, the original bug that mentioned disabling the whole combiner was:
https://llvm.org/PR22346

With more discussion here:
https://lists.llvm.org/pipermail/llvm-dev/2016-May/099686.html

[xgupta]

In D115808#3201928, @jrtc27 wrote:

In D115808#3201773, @xgupta wrote:

I will commit as it seems not a big change. and all comments are addressed.

This goes against the LLVM Code-Review Policy and Practices:

Code review can be an iterative process, which continues until the patch is ready to be committed. Specifically, once a patch is sent out for review, it needs an explicit approval before it is committed. Do not assume silent approval, or solicit objections to a patch with a deadline.

• https://llvm.org/docs/CodeReview.html#code-review-workflow

Sorry, I didn't read it before (I have seen a few times contributors commit in the such/same way) I will take care to not commit until patch is approved and all reviewers satisfy with the changes.

In D115808#3202913, @spatel wrote:

I agree that it would be better to have a higher-level fix for the problem and made a similar comment here:
D111596

But it doesn't seem like there's enough motivation to overcome whatever the underlying bugs are, so we have a bunch of these 'optnone' checks around DAGCombiner now.

Sorry, I read it fast that I didn't understand the meaning, I think you agree with the change, for now, don't want to revert it, right?

For reference, the original bug that mentioned disabling the whole combiner was:
https://llvm.org/PR22346

With more discussion here:
https://lists.llvm.org/pipermail/llvm-dev/2016-May/099686.html

I did read it before but not thoroughly I think it is fine to disable combining & canonicalization both if they affect debug info as mentioned in this mail by Robinson, Paul https://lists.llvm.org/pipermail/llvm-dev/2016-May/099772.html.

I doubt I can (may try) implement the registration mechanism that @qcolombet was talking about in that email thread.

[spatel]

In D115808#3204444, @xgupta wrote:

Sorry, I read it fast that I didn't understand the meaning, I think you agree with the change, for now, don't want to revert it, right?

Correct - based on previous patches/discussion, I see no reason to block this change.
It's not pretty to keep adding these checks, but there doesn't seem to be a viable alternative. This mess goes away as we move towards more functionality in GISel?

So LGTM, but please wait a day or so to re-commit in case others have more suggestions.

llvm/test/CodeGen/RISCV/optnone-store-no-combine.ll

5

"This test verifies that a repeated store is not eliminated with optnone (improves debugging)."

8

Remove dso_local, noinline, nounwind?

[xgupta]

test case update

[xgupta]

llvm/test/CodeGen/RISCV/optnone-store-no-combine.ll

8

After removing the noinline attribute I am getting

-- Testing: 1 tests, 1 workers --
FAIL: LLVM :: CodeGen/RISCV/optnone-store-no-combine.ll (1 of 1)
******************** TEST 'LLVM :: CodeGen/RISCV/optnone-store-no-combine.ll' FAILED ********************
Script:
--
: 'RUN: at line 2';   /home/xgupta/llvm/llvm-project/build/bin/llc -mtriple=riscv64 -verify-machineinstrs < /home/xgupta/llvm/llvm-project/llvm/test/CodeGen/RISCV/optnone-store-no-combine.ll | /home/xgupta/llvm/llvm-project/build/bin/FileCheck /home/xgupta/llvm/llvm-project/llvm/test/CodeGen/RISCV/optnone-store-no-combine.ll
--
Exit Code: 2

Command Output (stderr):
--
Attribute 'optnone' requires 'noinline'!
void (i32*)* @foo
llc: error: '<stdin>': input module cannot be verified
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /home/xgupta/llvm/llvm-project/build/bin/FileCheck /home/xgupta/llvm/llvm-project/llvm/test/CodeGen/RISCV/optnone-store-no-combine.ll

--

********************
********************
Failed Tests (1):
  LLVM :: CodeGen/RISCV/optnone-store-no-combine.ll


Testing Time: 1.67s
  Failed: 1

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (Ld->getBasePtr() == Ptr && ST->getMemoryVT() == Ld->getMemoryVT() &&
       return Chain;
     }
   }
 
   // TODO: Can relax for unordered atomics (see D66309)
   if (StoreSDNode *ST1 = dyn_cast<StoreSDNode>(Chain)) {
     if (ST->isUnindexed() && ST->isSimple() &&
         ST1->isUnindexed() && ST1->isSimple()) {
-      if (ST1->getBasePtr() == Ptr && ST1->getValue() == Value &&
-          ST->getMemoryVT() == ST1->getMemoryVT() &&
+      if (OptLevel != CodeGenOpt::None && ST1->getBasePtr() == Ptr &&
+          ST1->getValue() == Value && ST->getMemoryVT() == ST1->getMemoryVT() &&
           ST->getAddressSpace() == ST1->getAddressSpace()) {
         // If this is a store followed by a store with the same value to the
         // same location, then the store is dead/noop.
         return Chain;
       }
 
       if (OptLevel != CodeGenOpt::None && ST1->hasOneUse() &&
           !ST1->getBasePtr().isUndef() &&

llvm/test/CodeGen/RISCV/optnone-store-no-combine.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv64 -verify-machineinstrs < %s | FileCheck %s
+
+; This test verifies that a repeated store is not eliminated with optnone (improves debugging).
+
+define void @foo(i32* %p) noinline optnone {
+; CHECK-LABEL: foo:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    li a1, 8
+; CHECK-NEXT:    sw a1, 0(a0)
+; CHECK-NEXT:    sw a1, 0(a0)
+; CHECK-NEXT:    ret
+  store i32 8, i32* %p, align 4
+  store i32 8, i32* %p, align 4
+  ret void
+}