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

[Philip Reames via llvm-commits]

reames added a comment.

Yuck, how have we managed to be this wrong for this long and not notice it?

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.

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.


================
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?  


http://reviews.llvm.org/D21041