This is an archive of the discontinued LLVM Phabricator instance.

[SimplifyCFG] Extend SimplifyResume to handle phi of trivial landing pad.
ClosedPublic

Authored by chenli on Nov 3 2015, 3:30 PM.

Details

Diff Detail

Event Timeline

chenli updated this revision to Diff 39125.Nov 3 2015, 3:30 PM
chenli retitled this revision from to [SimplifyCFG] Extend SimplifyResume to handle phi of trivial landing pad..
chenli updated this object.
chenli added reviewers: reames, majnemer.
chenli added a subscriber: llvm-commits.
reames added inline comments.Nov 3 2015, 3:45 PM
lib/Transforms/Utils/SimplifyCFG.cpp
2959

This is at the wrong point. We should be checking that the terminator is a unconditional branch before considering the basic block trivial.

majnemer added inline comments.Nov 3 2015, 4:31 PM
lib/Transforms/Utils/SimplifyCFG.cpp
2986

!TrivialUnwindBlocks.empty()

chenli updated this revision to Diff 39424.Nov 5 2015, 3:18 PM

Update w.r.t Philip's and David's comments.

majnemer added inline comments.Nov 5 2015, 6:04 PM
lib/Transforms/Utils/SimplifyCFG.cpp
3242

Please do not evaluate getNumIncomingValues each time through the loop, consider writing this like:

for (unsigned Idx = 0, End = PhiLPInst->getNumIncomingValues(); Idx != End; ++Idx) {
3295

Isn't this BB && pred_empty(BB) ? Also, the braces are superfluous.

test/Transforms/SimplifyCFG/bug-25299.ll
1

No FileCheck?

chenli added inline comments.Nov 5 2015, 9:50 PM
lib/Transforms/Utils/SimplifyCFG.cpp
3242

Will do. But I think the compiler should be able to optimize this.

3295

Will do.

test/Transforms/SimplifyCFG/bug-25299.ll
1

The only purpose of this test is to verify there's no crash. The other test should be able to catch miscompiles. But I can definitely add FileCheck if it's useful in general.

chenli updated this revision to Diff 39572.Nov 6 2015, 11:44 AM

First, fix the issues mentioned by David.

Second, the previous patch turned out to have a bug by running some of my own local tests. The patch tried to erase all unreachable landing pad blocks, but this invalidated the iteration of SimplifyCFG pass.

if (SimplifyCFG(&*BBIt++, TTI, BonusInstThreshold, AC)) {
  LocalChange = true;
  ++NumSimpl;
}

The next block has been iterated before processing the current block (in this case it is the resume block). If the next block turns out to be an unreachable landing pad block deleted by processing the resume block, the iteration will be broken. Therefore, we can only erase the current block in SimplifyCFG.

The updated patch fixes this problem, and modify the test’s block layout to catch this bug if it happens.

reames added inline comments.Nov 6 2015, 4:44 PM
lib/Transforms/Utils/SimplifyCFG.cpp
3221

Given how divergent the two cases have gotten, I think we should split the two cases. The code duplication is relatively minor compared to the simplification of the complexity.

3277

I think this can be more clearly written as:
if (PhiLPInst)

while (PhiLPInst->getBasicBlockIndex(TrivialBB) != -1)
    BB->removePredecessor(TrivialBB, false);

Right?

reames requested changes to this revision.Nov 6 2015, 4:44 PM
reames edited edge metadata.
This revision now requires changes to proceed.Nov 6 2015, 4:44 PM
chenli added inline comments.Nov 10 2015, 2:51 PM
lib/Transforms/Utils/SimplifyCFG.cpp
3221

Will do.

3277

Yes, but this won't exist after we separate the two cases :)

chenli updated this revision to Diff 39871.Nov 10 2015, 4:49 PM
chenli edited edge metadata.

Update patch w.r.t Philip's suggestions.

reames requested changes to this revision.Jan 4 2016, 3:45 PM
reames edited edge metadata.

All of my comments this time around are minor. I think we can probably get this in after one last round of cleanup.

p.s. Sorry for the prolonged period without response.

lib/Transforms/Utils/SimplifyCFG.cpp
3216

Stale comment.

3221

Naming wise, unique isn't quite right. You could have a phi early in the value chain with a modification of it being resumed. I think it would make a lot more sense to lift the check for landing pad here to form an else if...

3286

minor wording: "if it has no", to "since it has no"

3292

Hm, what about an empty phi in an unreachable block? I think you're probably guarded from getting here, but adding an assert to make that clear would be good.

This revision now requires changes to proceed.Jan 4 2016, 3:45 PM
chenli added inline comments.Jan 6 2016, 1:01 PM
lib/Transforms/Utils/SimplifyCFG.cpp
3292

I didn't quite get this. If the resume block has no predecessor and become unreachable, it should contains an empty phi. Do you mean I should assert the empty phi is there?

chenli updated this revision to Diff 44364.Jan 8 2016, 12:41 PM
chenli edited edge metadata.

Update patch w.r.t. Philip's comments.

reames accepted this revision.Jan 8 2016, 2:47 PM
reames edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jan 8 2016, 2:47 PM
chenli closed this revision.Jan 9 2016, 9:51 PM