[DAGCombiner] Do not remove the load of stored values when optimizations are disabled

[kzhuravl]

Summary

This combiner breaks debug experience and should not be run when optimizations are disabled.

For example:
int main() {
int j = 0;
j += 2;
if (j == 2) return 0;
return 5;
}
When debugging this code compiled in /O0, it should be valid to break at line "j+=2;" and edit the value of j. It should change the return value of the function.

[etienneb]

Tom, who is the right reviewer for this?

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

10002 ↗

(On Diff #54360)

nit: please indent this correctly.

[mamai]

Fixed indentation.

[qcolombet]

Hi,

Does any of the combine done here even make sense for O0?
At first glance, I would say no. In other words, you should be able to exit earlier from this function.

More generally, I believe the problem is widely spread in SDISel and although it is a reasonable approach to fix cases as we encounter them, I think we (will) lack a nice design to fix the problem properly.

Could a start a RFC on llvm-dev about how to block dag combines in O0?

One last comment, please add a test case demonstrating that the combine does not occur at O0 and occurs at O1 and above.

Cheers,
-Quentin

[mamai]

Added a test case showing that the optimization is done in /O1 and not in /O0. Also added the fix to 2 test cases that were broken by this patch since the code generated changed slightly in /O0.

[qcolombet]

Hi Marianne,

What is the link for the RFC on how to block dag combines in O0?

I didn’t see the thread on llvm-dev and, again, I believe that we need a more systematic approach to solve this problem.

Cheers,
-Quentin

[mamai]

Sorry, I haven't started it yet. I will do that right now!

[mamai]

Here is a link to the RFC thread: https://groups.google.com/forum/#!topic/llvm-dev/JRM2tlYlJ6I. The idea doesn't seem very popular.

[qcolombet]

Hi Marianne,

Given the feedback on the other thread, yes, please carry on with the current approach.

Couple of comments inlined.

Cheers,
-Quentin

test/CodeGen/ARM/dag-combine-ldst.ll

12 ↗

(On Diff #57203)

Run opt -instnamer on the test case to get rid of the [0-9]+ variables.

35 ↗

(On Diff #57203)

Add a newline.

[mamai]

Is my last change correct? I ran the opt -instnamer and added a new line as asked.

[qcolombet]

test/CodeGen/ARM/dag-combine-ldst.ll

9 ↗

(On Diff #57831)

Could you add more check lines?

The problem I have here is that r0 is not defined by a value coming from the ABI and technically anything could feed that r0 register.
I.e., the test could pass because we generate add r1, r1 for instance. Therefore, please make sure the input register for the add is defined by the instruction we expect.

test/CodeGen/SystemZ/swift-return.ll

55 ↗

(On Diff #57831)

Where did the lgr instruction go?

[mamai]

test/CodeGen/SystemZ/swift-return.ll

55 ↗

(On Diff #57831)

It looks like la %[[REG1:r[0-9]+]], 168(%r15) and lgr %r2, %[[REG1]] have been merged into la %r2, 168(%r15). I supposed it was a side effect of my change, not an issue. But I am not familiar with SystemZ assembly, so I could be wrong.

[mamai]

Updated diff adding more checks in dag-combine-ldst.ll to make sure the %r0 comes from the right place.

Quentin, could you please tell me if it corresponds to what you had in mind? And by the way, thanks a lot for your time.

[qcolombet]

test/CodeGen/ARM/dag-combine-ldst.ll

8 ↗

(On Diff #58811)

Instead of hardcoding r0, use [[TMP:r[0-9]+]] then reuse [[TMP]].
That will make the test robust against regalloc changes.

10 ↗

(On Diff #58811)

Factor out the first three lines that are common to both patterns and add the check prefix to the command line.

I.e.,
; CHECK_O1: mov r0, #0
; CHECK_O1-NEXT: str r0, [sp, #4]
; CHECK_O1-NEXT: str r0, [sp]
; CHECK_O1-NOT: ldr r0, [sp]
; CHECK_O1-NOT: add r0, r0, #2

>

; CHECK: mov r0, #0
; CHECK-NEXT: str r0, [sp, #4]
; CHECK-NEXT: str r0, [sp]

[CHECK specific to O0 under CHECK_O0]

[CHECK specific to O1 under CHECK_O1]

;
RUN: […] —check-prefix=CHECK_O0

>

RUN: […] —check-prefix=CHECK_O0 —check-prefix=CHECK_O1

test/CodeGen/SystemZ/swift-return.ll

55 ↗

(On Diff #58811)

Could you have someone of the SystemZ backend to check.

[kzhuravl]

Hi Marianne,

We are hitting exactly the same issue in one of our (amdgpu) debugger test cases.
Are you still working on this patch?

Thanks,
Konstantin

[mamai]

Hi Konstantin,

I stopped working on it, since I am actually in maternity leave. I didn't had the time to fix the last issues mentionned by Quentin about the test files. Would you like to contribute? I am adding Cameron to the subscribers, he is a co-worker and can probably do the follow-up.

Marianne

[kzhuravl]

@mamai Thanks, I will update the patch

[kzhuravl]

Address review feedback

[kzhuravl]

Hi, @uweigand , this patch changes one of the SystemZ tests, I am not familiar with it. Who would be the best person to take a look at the SystemZ test changes?

thanks

[uweigand]

In D19268#558813, @kzhuravl wrote:

Hi, @uweigand , this patch changes one of the SystemZ tests, I am not familiar with it. Who would be the best person to take a look at the SystemZ test changes?

This change is fine, the new assembler code is actually better than the old one.

[kzhuravl]

Thanks, @uweigand !

[kzhuravl]

Ping

[qcolombet]

test/CodeGen/ARM/dag-combine-ldst.ll

2 ↗

(On Diff #73248)

You'll need to add -check-prefix=CHECK as well, otherwise the "plain" CHECK lines won't be checked.

[kzhuravl]

Address review comments

test/CodeGen/ARM/dag-combine-ldst.ll

2 ↗

(On Diff #73248)

I did not know that. Thanks!

[qcolombet]

LGTM.

[kzhuravl]

Thanks @qcolombet !

llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (N->getValueType(1) == MVT::Other) {
       }
     }
   }
 
   // If this load is directly stored, replace the load value with the stored
   // value.
   // TODO: Handle store large -> read small portion.
   // TODO: Handle TRUNCSTORE/LOADEXT
-  if (ISD::isNormalLoad(N) && !LD->isVolatile()) {
+  if (OptLevel != CodeGenOpt::None &&
+      ISD::isNormalLoad(N) && !LD->isVolatile()) {
     if (ISD::isNON_TRUNCStore(Chain.getNode())) {
       StoreSDNode *PrevST = cast<StoreSDNode>(Chain);
       if (PrevST->getBasePtr() == Ptr &&
           PrevST->getValue().getValueType() == N->getValueType(0))
       return CombineTo(N, Chain.getOperand(1), Chain);
     }
   }
 

llvm/trunk/test/CodeGen/ARM/alloc-no-stack-realign.ll

 ; NO-REALIGN: vld1.64 {{{d[0-9]+, d[0-9]+}}}, [r[[R2]]:128]
 ; NO-REALIGN: add r[[R2:[0-9]+]], r[[R1]], #48
 ; NO-REALIGN: vld1.64 {{{d[0-9]+, d[0-9]+}}}, [r[[R2]]:128]
 
 ; NO-REALIGN: add r[[R2:[0-9]+]], r[[R1:[0-9]+]], #48
 ; NO-REALIGN: vst1.64 {{{d[0-9]+, d[0-9]+}}}, [r[[R2]]:128]
 ; NO-REALIGN: add r[[R2:[0-9]+]], r[[R1]], #32
 ; NO-REALIGN: vst1.64 {{{d[0-9]+, d[0-9]+}}}, [r[[R2]]:128]
-; NO-REALIGN: vst1.32 {{{d[0-9]+, d[0-9]+}}}, [r[[R1]]:128]!
-; NO-REALIGN: vst1.64 {{{d[0-9]+, d[0-9]+}}}, [r[[R1]]:128]
+; NO-REALIGN: mov r[[R3:[0-9]+]], r[[R1]]
+; NO-REALIGN: vst1.32 {{{d[0-9]+, d[0-9]+}}}, [r[[R3]]:128]!
+; NO-REALIGN: vst1.64 {{{d[0-9]+, d[0-9]+}}}, [r[[R3]]:128]
 
 ; NO-REALIGN: add r[[R2:[0-9]+]], r[[R0:0]], #48
 ; NO-REALIGN: vst1.64 {{{d[0-9]+, d[0-9]+}}}, [r[[R2]]:128]
 ; NO-REALIGN: add r[[R2:[0-9]+]], r[[R0]], #32
 ; NO-REALIGN: vst1.64 {{{d[0-9]+, d[0-9]+}}}, [r[[R2]]:128]
 ; NO-REALIGN: vst1.32 {{{d[0-9]+, d[0-9]+}}}, [r[[R0]]:128]!
 ; NO-REALIGN: vst1.64 {{{d[0-9]+, d[0-9]+}}}, [r[[R0]]:128]
  %retval = alloca <16 x float>, align 16

llvm/trunk/test/CodeGen/ARM/dag-combine-ldst.ll

+; RUN: llc < %s -mtriple=arm-eabi -mattr=+v4t -O0 | FileCheck %s -check-prefix=CHECK -check-prefix=CHECK_O0
+; RUN: llc < %s -mtriple=arm-eabi -mattr=+v4t -O1 | FileCheck %s -check-prefix=CHECK -check-prefix=CHECK_O1
+
+; In /O0, the addition must not be eliminated. This happens when the load
+; and store are folded by the DAGCombiner. In /O1 and above, the optimization
+; must be executed.
+
+; CHECK-LABEL:   {{^}}main
+; CHECK:         mov [[TMP:r[0-9]+]], #0
+; CHECK-NEXT:    str [[TMP]], [sp, #4]
+; CHECK-NEXT:    str [[TMP]], [sp]
+; CHECK_O0:      ldr [[TMP:r[0-9]+]], [sp]
+; CHECK_O0-NEXT: add [[TMP]], [[TMP]], #2
+; CHECK_O1-NOT:  ldr [[TMP:r[0-9]+]], [sp]
+; CHECK_O1-NOT:  add [[TMP]], [[TMP]], #2
+
+define i32 @main() {
+bb:
+  %tmp = alloca i32, align 4
+  %tmp1 = alloca i32, align 4
+  store i32 0, i32* %tmp, align 4
+  store i32 0, i32* %tmp1, align 4
+  %tmp2 = load i32, i32* %tmp1, align 4
+  %tmp3 = add nsw i32 %tmp2, 2
+  store i32 %tmp3, i32* %tmp1, align 4
+  %tmp4 = load i32, i32* %tmp1, align 4
+  %tmp5 = icmp eq i32 %tmp4, 2
+  br i1 %tmp5, label %bb6, label %bb7
+
+bb6:                                              ; preds = %bb
+  store i32 0, i32* %tmp, align 4
+  br label %bb8
+
+bb7:                                              ; preds = %bb
+  store i32 5, i32* %tmp, align 4
+  br label %bb8
+
+bb8:                                              ; preds = %bb7, %bb6
+  %tmp9 = load i32, i32* %tmp, align 4
+  ret i32 %tmp9
+}

llvm/trunk/test/CodeGen/SystemZ/swift-return.ll

 ; CHECK-DAG: lr %r3, %[[REG1]]
 ; CHECK: brasl %r14, gen2
 ; CHECK: l %r2, 160(%r15)
 ; CHECK: a %r2, 164(%r15)
 ; CHECK: a %r2, 168(%r15)
 ; CHECK: a %r2, 172(%r15)
 ; CHECK: a %r2, 176(%r15)
 ; CHECK-O0-LABEL: test2:
-; CHECK-O0: la %[[REG1:r[0-9]+]], 168(%r15)
 ; CHECK-O0: st %r2, [[SPILL1:[0-9]+]](%r15)
-; CHECK-O0: lgr %r2, %[[REG1]]
 ; CHECK-O0: l %r3, [[SPILL1]](%r15)
+; CHECK-O0: la %r2, 168(%r15)
 ; CHECK-O0: brasl %r14, gen2
 ; CHECK-O0-DAG: l %r{{.*}}, 184(%r15)
 ; CHECK-O0-DAG: l %r{{.*}}, 180(%r15)
 ; CHECK-O0-DAG: l %r{{.*}}, 176(%r15)
 ; CHECK-O0-DAG: l %r{{.*}}, 172(%r15)
 ; CHECK-O0-DAG: l %r{{.*}}, 168(%r15)
 ; CHECK-O0: ar
 ; CHECK-O0: ar

llvm/trunk/test/CodeGen/X86/inline-asm-tied.ll

 ; RUN: llc < %s -mtriple=i386-apple-darwin9 -O0 -optimize-regalloc -regalloc=basic -no-integrated-as | FileCheck %s
 ; rdar://6992609
 
-; CHECK: movl [[EDX:%e..]], 4(%esp)
-; CHECK: movl [[EDX]], 4(%esp)
+; CHECK: movl %ecx, 4([[ESP:%e..]])
+; CHECK: movl 4([[ESP]]), [[EDX:%e..]]
+; CHECK: movl [[EDX]], 4([[ESP]])
 target triple = "i386-apple-darwin9.0"
 @llvm.used = appending global [1 x i8*] [i8* bitcast (i64 (i64)* @_OSSwapInt64 to i8*)], section "llvm.metadata"    ; <[1 x i8*]*> [#uses=0]
 
 define i64 @_OSSwapInt64(i64 %_data) nounwind {
 entry:
   %retval = alloca i64    ; <i64*> [#uses=2]
   %_data.addr = alloca i64    ; <i64*> [#uses=4]
   store i64 %_data, i64* %_data.addr

llvm/trunk/test/CodeGen/X86/pr30430.ll

 ; CHECK-NEXT:    vmovss {{.*#+}} xmm0 = mem[0],zero,zero,zero
 ; CHECK-NEXT:    vmovss {{.*#+}} xmm1 = mem[0],zero,zero,zero
 ; CHECK-NEXT:    vmovss {{.*#+}} xmm2 = mem[0],zero,zero,zero
 ; CHECK-NEXT:    vmovss {{.*#+}} xmm3 = mem[0],zero,zero,zero
 ; CHECK-NEXT:    vmovss {{.*#+}} xmm4 = mem[0],zero,zero,zero
 ; CHECK-NEXT:    vmovss {{.*#+}} xmm5 = mem[0],zero,zero,zero
 ; CHECK-NEXT:    vmovss {{.*#+}} xmm6 = mem[0],zero,zero,zero
 ; CHECK-NEXT:    vmovss {{.*#+}} xmm7 = mem[0],zero,zero,zero
+; CHECK-NEXT:    vmovss {{.*#+}} xmm8 = mem[0],zero,zero,zero
 ; CHECK-NEXT:    vmovss {{.*#+}} xmm9 = mem[0],zero,zero,zero
 ; CHECK-NEXT:    vmovss {{.*#+}} xmm10 = mem[0],zero,zero,zero
 ; CHECK-NEXT:    vmovss {{.*#+}} xmm11 = mem[0],zero,zero,zero
 ; CHECK-NEXT:    vmovss {{.*#+}} xmm12 = mem[0],zero,zero,zero
 ; CHECK-NEXT:    vmovss {{.*#+}} xmm13 = mem[0],zero,zero,zero
 ; CHECK-NEXT:    vmovss {{.*#+}} xmm14 = mem[0],zero,zero,zero
 ; CHECK-NEXT:    vmovss {{.*#+}} xmm15 = mem[0],zero,zero,zero
-; CHECK-NEXT:    vmovss %xmm8, {{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    vmovss %xmm0, {{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    vmovss %xmm1, {{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    vmovss %xmm2, {{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    vmovss %xmm3, {{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    vmovss %xmm4, {{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    vmovss %xmm5, {{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    vmovss %xmm6, {{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    vmovss %xmm7, {{[0-9]+}}(%rsp)
+; CHECK-NEXT:    vmovss %xmm8, {{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    vmovss %xmm9, {{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    vmovss %xmm10, {{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    vmovss %xmm11, {{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    vmovss %xmm12, {{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    vmovss %xmm13, {{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    vmovss %xmm14, {{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    vmovss %xmm15, {{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    vmovss {{.*#+}} xmm0 = mem[0],zero,zero,zero

 ; CHECK-NEXT:    vmovss {{.*#+}} xmm0 = mem[0],zero,zero,zero
 ; CHECK-NEXT:    vmovss {{.*#+}} xmm1 = mem[0],zero,zero,zero
 ; CHECK-NEXT:    vinsertps {{.*#+}} xmm0 = xmm1[0],xmm0[0],xmm1[2,3]
 ; CHECK-NEXT:    vmovss {{.*#+}} xmm1 = mem[0],zero,zero,zero
 ; CHECK-NEXT:    vinsertps {{.*#+}} xmm0 = xmm0[0,1],xmm1[0],xmm0[3]
 ; CHECK-NEXT:    vmovss {{.*#+}} xmm1 = mem[0],zero,zero,zero
 ; CHECK-NEXT:    vinsertps {{.*#+}} xmm0 = xmm0[0,1,2],xmm1[0]
 ; CHECK-NEXT:    vmovss {{.*#+}} xmm1 = mem[0],zero,zero,zero
-; CHECK-NEXT:    vinsertps {{.*#+}} xmm1 = xmm15[0],xmm1[0],xmm15[2,3]
+; CHECK-NEXT:    vmovss {{.*#+}} xmm3 = mem[0],zero,zero,zero
+; CHECK-NEXT:    vinsertps {{.*#+}} xmm1 = xmm3[0],xmm1[0],xmm3[2,3]
 ; CHECK-NEXT:    vmovss {{.*#+}} xmm3 = mem[0],zero,zero,zero
 ; CHECK-NEXT:    vinsertps {{.*#+}} xmm1 = xmm1[0,1],xmm3[0],xmm1[3]
 ; CHECK-NEXT:    vmovss {{.*#+}} xmm3 = mem[0],zero,zero,zero
 ; CHECK-NEXT:    vinsertps {{.*#+}} xmm1 = xmm1[0,1,2],xmm3[0]
 ; CHECK-NEXT:    # implicit-def: %YMM3
 ; CHECK-NEXT:    vmovaps %xmm1, %xmm3
 ; CHECK-NEXT:    vinsertf128 $1, %xmm0, %ymm3, %ymm3
 ; CHECK-NEXT:    # implicit-def: %ZMM16