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

[Anna Thomas via Phabricator via llvm-commits]

anna requested changes to this revision.
anna added a comment.
This revision now requires changes to proceed.

Good find Yevgeny! 
So, `removeUnreachableBlocks` is much more aggressive than `DT.isReachableFromEntry`. Did you check if there's some function that just keeps exactly what `isReachableFromEntry` computes?

Other comments inline.



================
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.
----------------
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`. 


================
Comment at: test/Transforms/RewriteStatepointsForGC/unreachable-regression.ll:7
+;   RewriteStatepointsForGC called removeUnreachableBlocks().
+;   removeUnreachableBlocks() could remove some reachable blocks with the
+;   collected rewritable callsites. This made the callsites invalid and
----------------
This was very confusing "why would removeUnreachableBlocks remove reachable blocks :)". 

Once the unconditional canonicalization step is done, pls modify this comment and state that "Parse points are calculated only for reachable blocks". 




Repository:
  rL LLVM

https://reviews.llvm.org/D43929