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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:2049
       ChangedFunction |= replaceOperandsWithConsts(&*BI);
+    if (IsGuard(*BI))
+      BlocksWithGuards.insert(BB);
----------------
dberlin wrote:
> 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.
> 
Yes, we only can rely on this set if we have visited all the predecessors before. This is true for the case where we use it (because there is only 1 predecessor) and not true in general. I'll write a comment on that to make it clear.


https://reviews.llvm.org/D37460