This is an archive of the discontinued LLVM Phabricator instance.

PredicateInfo: Support switch statements
ClosedPublic

Authored by dberlin on Feb 8 2017, 8:38 PM.

Details

Summary

Depends on D29606 and D29682

Makes us pass GVN's edge.ll (we also will pass a few other testcases
they just need cleaning up).

Thoughts on the Predicate* hiearchy of classes especially welcome :)
(it's not clear to me how best to organize it, and currently, the getBlock* seems ... uglier than maybe wasting a field somewhere or something).

Diff Detail

Repository
rL LLVM

Event Timeline

dberlin created this revision.Feb 8 2017, 8:38 PM
dberlin added inline comments.Feb 9 2017, 8:54 PM
lib/Transforms/Scalar/NewGVN.cpp
862 ↗(On Diff #87755)

sigh, clang format, i'll fix

876 ↗(On Diff #87755)

ditto

880 ↗(On Diff #87755)

ditto

  • Update new tests
dberlin marked 3 inline comments as done.Feb 11 2017, 5:22 AM
dberlin added inline comments.
lib/Transforms/Scalar/NewGVN.cpp
845 ↗(On Diff #88091)

See comment below about branch having two parents.
The switch code is the only real new code in this function,

1061 ↗(On Diff #88091)

Just a friendly note: This branch has two parents (the critical edge patch and the initial newgvn patch).
So this diff shows a lot more than is actually here.
In particular, this function is not really being added here.

davide requested changes to this revision.Feb 12 2017, 12:30 PM

Once the basic predicate support goes in, can you please rebase this patch so I can review it more easily? Thanks!

This revision now requires changes to proceed.Feb 12 2017, 12:30 PM

Yup, as soon as those two are committed i'll rebase and ping you.

dberlin edited edge metadata.

Rebase on top of master and clean up.

davide added inline comments.Feb 21 2017, 10:22 AM
lib/Transforms/Scalar/NewGVN.cpp
1152 ↗(On Diff #89056)

Spurious newline

lib/Transforms/Utils/PredicateInfo.cpp
56–59 ↗(On Diff #89056)

assert(isa<>) + return static_cast<> ?

65–67 ↗(On Diff #89056)

Also here and in the other places.

442–450 ↗(On Diff #89056)

range for?

dberlin marked 2 inline comments as done.
  • Update for review comments

Note: This adds two pair conversion function for BasicBlockEdge in
dominators to make life easier. We can't just use basicblockedge
ourselves, because we need non-const basic blocks in a few places.

davide accepted this revision.Feb 22 2017, 11:37 AM

LGTM. Thanks.

This revision is now accepted and ready to land.Feb 22 2017, 11:37 AM
This revision was automatically updated to reflect the committed changes.