Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

Have isKnownNotFullPoison be smarter around control flow
ClosedPublic

Authored by sanjoy on Apr 17 2016, 8:52 PM.

Details

Summary

(... while still not using a PostDomTree)

The way we use isKnownNotFullPoison from SCEV today, the new CFG walking
logic will not trigger for any realistic cases -- it will kick in only
for situations where we could have merged the contiguous basic blocks
anyway[0], since the poison generating instruction dominates all of its
non-PHI uses (which are the only uses we consider right now).

However, having this change in place will allow a later bugfix to break
fewer llvm-lit tests.

[0]: i.e. cases where block A branches to block B and B is A's only
successor and A is B's only predecessor.

Diff Detail

Repository
rL LLVM

Event Timeline

sanjoy updated this revision to Diff 54029.Apr 17 2016, 8:52 PM
sanjoy retitled this revision from to Have isKnownNotFullPoison be smarter around control flow.
sanjoy updated this object.
sanjoy added reviewers: broune, bjarke.roune.
sanjoy added a subscriber: llvm-commits.
broune accepted this revision.Apr 21 2016, 5:14 PM
broune edited edge metadata.
broune added inline comments.
lib/Analysis/ValueTracking.cpp
3413 ↗(On Diff #54029)

As far as I can tell, this code for Br is not in the official repository, yet it is not listed as a changed line in the review.

https://github.com/llvm-mirror/llvm/blob/615a6cf40aa9c7b280fd665e4ad66d4e0d5395fe/lib/Analysis/ValueTracking.cpp#L3522

Maybe I'm just confused about how diffs are shown or this was added very recently?

3442 ↗(On Diff #54029)

Would it make sense to have a limit on how many instructions/basic blocks this will consider?

3462 ↗(On Diff #54029)

Could there be braces for this multi-line if?

This revision is now accepted and ready to land.Apr 21 2016, 5:14 PM
sanjoy added inline comments.Apr 21 2016, 5:18 PM
lib/Analysis/ValueTracking.cpp
3413 ↗(On Diff #54029)

This is my fault. I had this revision based off of D19211 but didn't note it in the revision.

sanjoy marked an inline comment as done.Apr 21 2016, 11:39 PM
sanjoy added inline comments.
lib/Analysis/ValueTracking.cpp
3442 ↗(On Diff #54029)

Added a limit of MaxDepth blocks (for the lack of a better limit).

This revision was automatically updated to reflect the committed changes.