[PATCH] D37460: [GVN] Prevent LoadPRE from hoisting through guards

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added inline comments.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:2049
       ChangedFunction |= replaceOperandsWithConsts(&*BI);
+    if (IsGuard(*BI))
+      BlocksWithGuards.insert(BB);
----------------
reames wrote:
> I'm not sure building this set in RPO is sufficient in the face of cycles.  I believe the current PRE code would bail out of said cycles, but at minimum, that assumptions needs to be carefully documented and asserted for.
> 
> i.e. how do we know that BlocksWithGuards has been populated for each block we traverse backwards through?
> 
> In particular, if we extend this to isGuaranteedTo... and the scalar PRE case from https://reviews.llvm.org/D21041, it's not obvious to me this code would be correct.
It's **definitely** not sufficient in the case of cycles.

The only reason it's thought to be sufficient here is that you are guaranteed that any straight line predecessors have already been processed.

Since it's only walking back through straight line predecessors, ...

So yes, if it tried to use this info in random predecessors, it would not be correct the way it is being computed here.



https://reviews.llvm.org/D37460