[PATCH] D43929: [RewriteStatepoints] Fix stale parse points

[Anna Thomas via Phabricator via llvm-commits]

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.
----------------
yrouban wrote:
> 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);;
+  DD.flush();
----------------
Nit: extra semicolon.


https://reviews.llvm.org/D43929