This is an archive of the discontinued LLVM Phabricator instance.

[SimplifyCFG] Fold `llvm.guard(false)` to unreachable
ClosedPublic

Authored by sanjoy on Apr 18 2016, 3:54 PM.

Details

Summary

llvm.guard(false) always bails out of the current compilation unit, so
we can prune any control flow following it.

Note: earlier I had submitted an incorrect version of this change as
D19188.

Diff Detail

Event Timeline

sanjoy updated this revision to Diff 54135.Apr 18 2016, 3:54 PM
sanjoy retitled this revision from to [SimplifyCFG] Fold `llvm.guard(false)` to unreachable.
sanjoy updated this object.
sanjoy added reviewers: hfinkel, pcc.
sanjoy added a subscriber: llvm-commits.
reames added a subscriber: reames.Apr 20 2016, 11:34 AM
reames added inline comments.
include/llvm/Transforms/Utils/Local.h
295 ↗(On Diff #54135)

Why can't you just use the API immediately above?

lib/Transforms/Utils/Local.cpp
1279

You should respect UseLLVMTrap here.

1330

Add a comment explain what this does rather than what it does not do.

sanjoy updated this revision to Diff 54402.Apr 20 2016, 12:25 PM
sanjoy marked 2 inline comments as done.
  • Address review
include/llvm/Transforms/Utils/Local.h
295 ↗(On Diff #54135)

No specific reason; just from a readability POV I found it a little better to have a separately named utility than add a bool to an existing one. If you prefer editing changeToUnreachable instead, I can do that too (as you can tell, I won't even have to change the implementation).

In case you were asking why I cannot use the API above as is, changeToUnreachable removes I. But for guards the original instruction needs to remain, since that is how we'll get out of the compile (IOW it is more like "exit(0)" than it is like "store to null").

reames added inline comments.Apr 20 2016, 5:10 PM
include/llvm/Transforms/Utils/Local.h
295 ↗(On Diff #54402)

I'm still missing something here. I get why you can't remove the *guard*, but why can't you remove the instruction *after* the guard? changeToUnreachable(std::next(BBI))?

sanjoy updated this revision to Diff 54445.Apr 20 2016, 5:32 PM

You're right, there was never a need for a new utility. Fixed.

majnemer added inline comments.
lib/Transforms/Utils/Local.cpp
1331

An call -> A call.

1341

I think the usual style doesn't have spaces before the variable or around the equals:

changeToUnreachable(II->getNextNode(), /*UseLLVMTrap=*/false);

I've also seen it formatted like so:

changeToUnreachable(II->getNextNode(), /*UseLLVMTrap*/ false);
reames accepted this revision.Apr 20 2016, 8:58 PM
reames added a reviewer: reames.

LGTM w/remaining comments from David and my own addressed.

test/Transforms/SimplifyCFG/guards.ll
12

I believe this branch is redundant?

23

Clarify: and do not want to.

Also, add a test case for guard(true) just as a sanity check!

45

Naming wise: true, false are confusing. Possibly: dead_via_guard and merge? Applies to all tests.

This revision is now accepted and ready to land.Apr 20 2016, 8:58 PM
This revision was automatically updated to reflect the committed changes.
sanjoy marked 3 inline comments as done.