[PATCH] D43929: [RewriteStatepoints] Fix stale parse points
Anna Thomas via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 1 07:32:38 PST 2018
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".
More information about the llvm-commits