This is an archive of the discontinued LLVM Phabricator instance.

[SimplifyCFG] isSafeToSpeculateStore now ignores debug info
ClosedPublic

Authored by Henric on May 4 2016, 4:07 AM.

Details

Summary

This patch fixes PR27615
@llvm.dbg.value instructions no longer count towards the maximum number of instructions to look back at in the instruction list when searching for a store instruction. This should make the output consistent between debug and non-debug build.

Diff Detail

Event Timeline

Henric updated this revision to Diff 56119.May 4 2016, 4:07 AM
Henric retitled this revision from to [SimplifyCFG] isSafeToSpeculateStore now ignores debug info.
Henric updated this object.
Henric added a reviewer: hans.
Henric added a subscriber: llvm-commits.

Minor comments on the test, but the change itself LGTM.

test/Transforms/SimplifyCFG/PR27615-simplify-cond-br.ll
1

If the expectation is that the output should be exactly the same with and without debug, I'd suggest dropping the prefixes from both RUN commands and have only a single version of the check. I.e.,

; CHECK: select i1 %or.cond, i32 -1, i32 5
; CHECK-NOT: bb1:
mcrosier requested changes to this revision.May 4 2016, 6:43 AM
mcrosier added a reviewer: mcrosier.
mcrosier added inline comments.
lib/Transforms/Utils/SimplifyCFG.cpp
1451

Actually, in the original code we were doing a pre-decrement prior to the check. Now the decrement happens after the check, so I believe we're now doing one extra iteration of the loop.

This revision now requires changes to proceed.May 4 2016, 6:43 AM
Henric added inline comments.May 4 2016, 7:07 AM
lib/Transforms/Utils/SimplifyCFG.cpp
1451

Well spotted, I'll fix a patch where the initial count is reduced too so the behaviour is unchanged.

test/Transforms/SimplifyCFG/PR27615-simplify-cond-br.ll
1

The reason why I made different checks, was to make it easier to see which run that failed. But maybe it's better to see that the tests are actually equal.

mcrosier added inline comments.May 4 2016, 7:21 AM
lib/Transforms/Utils/SimplifyCFG.cpp
1451

Thanks.

test/Transforms/SimplifyCFG/PR27615-simplify-cond-br.ll
1

I'd prefer a single check, but I'll let you make the call as it makes very little differences and is a subjective call.

Henric updated this revision to Diff 56149.May 4 2016, 7:44 AM
Henric edited edge metadata.

-MaxNumInstToLookAt starts at 9 to account for the move of pre decrement
-Both test runs use same checks, to make it more clear that the output should be the same

mcrosier accepted this revision.May 4 2016, 7:49 AM
mcrosier edited edge metadata.

LGTM.

This revision is now accepted and ready to land.May 4 2016, 7:49 AM
hans accepted this revision.May 4 2016, 8:18 AM
hans edited edge metadata.

lgtm

Do you have commit access, or would you like me to commit this for you?

Henric added a comment.May 4 2016, 8:23 AM

I don't have commit access, so if you can do the commit for me that would be nice. Thanks.

This revision was automatically updated to reflect the committed changes.