[PATCH] D43929: [RewriteStatepoints] Fix stale parse points
Anna Thomas via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Mar 3 07:50:53 PST 2018
anna accepted this revision.
anna added a comment.
This revision is now accepted and ready to land.
LGTM w/ comments.
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2534
// consider those in reachable code since we need to ask dominance queries
- // when rewriting. We'll delete the unreachable ones in a moment.
+ // when rewriting. If unreachable found then we remove all unreachable
+ // blocks and recollect the statepoints again.
> anna wrote:
> > I'm not a fan of double collection of statepoints :) How about making this clearer:
> > 1. Unconditionally run canonicalizing step: `removeUnreachableBlocks`.
> > 2. Compute statepoints using line 2561 - 2565. At this point, just an assert that `DT.isReachableFromEntry` is enough.
> > Also, pls add an explicit comment stating that `removeUnreachableBlocks` is stronger than `isReachableFromEntry`.
> Ok. Make sense.
> I'm not sure if removeUnreachanbleBlocks() is easy. That is why I propose its lazy run. I will make it simple as you suggested.
Note that apart from canonicalizing the IR, we also got rid of the `isReachableFromEntry` check on every parse point.
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2535
+ // statepoints surviving this pass. This makes testing easier and the
+ // resulting IR less confusing to human readers. Rather than be fancy, we
+ // just reuse a utility function which removes the unreachable blocks.
I don't think this last line is needed here. `removeUnreachableBlocks` seems to be rather fancy compared to isReachableFromEntry ;)
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2538
+ DeferredDominance DD(DT);
+ bool MadeChange = removeUnreachableBlocks(F, nullptr, &DD);;
Nit: extra semicolon.
More information about the llvm-commits