[PATCH] D21041: [GVN] PRE can't hoist loads across calls in general.

[Eli Friedman via llvm-commits]

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()))
----------------
reames wrote:
> 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.


http://reviews.llvm.org/D21041