[PATCH] D21041: [GVN] PRE can't hoist loads across calls in general.
Eli Friedman via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 13 23:59:02 PDT 2016
eli.friedman added a comment.
In http://reviews.llvm.org/D21041#457240, @reames wrote:
> Yuck, how have we managed to be this wrong for this long and not notice it?
For the load case, you have to have a pointer which is both NoAlias relative to an arbitrary call and not dereferenceable... so it's basically impossible to trigger at the moment unless you have noalias metadata.
> Like Danny, I'm hesitant to add the instruction walks here. MDA already did that once, so doing the walk again seems very wasteful. I can see a couple of options here:
> 1. Add an analysis which tracks where a basic block always exits if entered. By returning a safe result ("maybe?"), the invalidation wouldn't be too painful. This would be a step in a direction I've been thinking about for a while to consolidate all of our various dereferenceability checks.
> 2. Extend MDA to track this information when doing it's walk. This would be strictly weaker than the current code though.
See also http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20160613/364534.html
I'm hesitant to add a new analysis pass because keeping the analysis up-to-date is inevitably painful... especially given that GVN probably wants to check this at higher resolution than just per-block. Maybe it's the right approach, though.
> At minimum, the patch should be rewritten as an extension to isValueFullyAvailableInBlock; that's the bit that's supposed to be reasoning about anticipation for LoadPRE.
I'm not following; this isn't something we need to check on a per-predecessor basis. The issue is basically whether it's legal to hoist a given load from the middle of its parent BB to the beginning of that BB.
Comment at: lib/Transforms/Scalar/GVN.cpp:2422
@@ -2403,1 +2421,3 @@
+ if (!isSafeToSpeculativelyExecute(CurInst) &&
+ !canHoistAcross(CurrentBlock->begin(), CurInst->getIterator()))
> Can you point to the test case which requires this part? ScalarPRE is not supposed to need to reason about availability. Where does this break?
> Also, can this be rephrased in terms of the same isValueFullyAvailableInBlock helper?
See testcase; this is basically just avoiding dividing by zero.
More information about the llvm-commits