[PATCH] D37460: [GVN] Prevent LoadPRE from hoisting across instructions that don't pass control flow to successors

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:2189
 
+  // Make sure that there are no instructions with implicit control flow that
+  // could prevent us from reaching our instruction.
----------------
reames wrote:
> mkazantsev wrote:
> > mkazantsev wrote:
> > > reames wrote:
> > > > This is overly conservative for any instruction which can not ever fault.  For instance, PRE a load over a throwing call is perfectly sound.  Note that preserving the nsw/nuw flags is questionable in the same case, but that's a hard discussion.  :)
> > > > 
> > > > I'd prefer to see the scalarPRE part separate into it's own patch.  I know I'm saying the exact opposite of Eli here, but I think this needs more discussion and trying to combine them will delay the loadPRE patch unnecessarily.  
> > > I disagree that PRE over a throwing call is invalid. Imagine the following situation:
> > > 
> > >   int arr[LEN];
> > > 
> > >   void foo(index) {
> > >     ...
> > >     call rangeCheck(index, LEN);
> > >     %x = load %arr, %index
> > >     print(%x);
> > >   }
> > > 
> > >   void rangeCheck(int index, int len) {
> > >     if (index < 0 || index >= len)
> > >       throw exception;
> > >   }
> > > 
> > > We cannot PRE across the throwing call here.
> > >> I disagree that prohibiting PRE over a throwing call is invalid
> > 
> > :)
> Sorry, I should have said "PRE of a *known safe to speculate* load over a throwing call is perfectly sound".  My sloppy wording here was really confusing, sorry!
>I'd prefer to see the scalarPRE part separate into it's own patch. I know I'm saying the exact opposite of Eli here, but I think this needs more discussion and trying to combine them will delay the loadPRE patch unnecessarily.

I mostly pointed out the issue with scalars here because it makes sense to write the patches together; they're very similar issues, and the solution requires the same infrastructure.  If you want to separate it out for review/bisectability, that's fine.

And yes, we should probably be checking for isSafeToSpeculativelyExecute(), for both scalar and load PRE.


https://reviews.llvm.org/D37460