Page MenuHomePhabricator

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

Authored by kzhuravl on Apr 19 2016, 8:58 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

mamai updated this revision to Diff 54199.Apr 19 2016, 8:58 AM
mamai retitled this revision from to [DAGCombiner] Skip folding constants when optimizations are disabled.
mamai updated this object.
mamai added a reviewer: resistor.
mamai set the repository for this revision to rL LLVM.
mamai added a subscriber: llvm-commits.
mamai updated this revision to Diff 54360.Apr 20 2016, 6:23 AM
mamai retitled this revision from [DAGCombiner] Skip folding constants when optimizations are disabled to [DAGCombiner] Do not remove the load of stored values when optimizations are disabled.
etienneb added a subscriber: etienneb.

Tom, who is the right reviewer for this?

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10002 ↗(On Diff #54360)

nit: please indent this correctly.

mamai updated this revision to Diff 55827.May 2 2016, 8:54 AM
mamai removed rL LLVM as the repository for this revision.

Fixed indentation.

qcolombet edited edge metadata.May 2 2016, 10:26 AM

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 updated this revision to Diff 57203.May 13 2016, 8:58 AM
mamai edited edge metadata.
mamai set the repository for this revision to rL LLVM.

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.

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 added a comment.May 13 2016, 9:44 AM

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

mamai added a comment.May 16 2016, 7:10 AM

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

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 updated this revision to Diff 57831.May 19 2016, 11:46 AM
mamai added a comment.May 25 2016, 5:58 AM

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

qcolombet added inline comments.May 26 2016, 3:56 PM
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 added inline comments.May 27 2016, 10:15 AM
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 this revision to Diff 58811.May 27 2016, 11:08 AM

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 added inline comments.May 31 2016, 4:40 PM
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.

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

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 commandeered this revision.Oct 3 2016, 2:08 AM
kzhuravl added a reviewer: mamai.

@mamai Thanks, I will update the patch

kzhuravl updated this revision to Diff 73248.Oct 3 2016, 2:09 AM
kzhuravl removed rL LLVM as the repository for this revision.

Address review feedback

kzhuravl added a subscriber: uweigand.

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 edited edge metadata.Oct 4 2016, 3:39 AM

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 marked 3 inline comments as done.Oct 4 2016, 9:35 AM

Thanks, @uweigand !

qcolombet added inline comments.Oct 11 2016, 10:09 AM
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 updated this revision to Diff 74301.Oct 11 2016, 2:50 PM
kzhuravl edited edge metadata.
kzhuravl marked an inline comment as done.

Address review comments

test/CodeGen/ARM/dag-combine-ldst.ll
2 ↗(On Diff #73248)

I did not know that. Thanks!

qcolombet accepted this revision.Oct 11 2016, 3:57 PM
qcolombet edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Oct 11 2016, 3:57 PM
This revision was automatically updated to reflect the committed changes.