[PATCH] D29092: PR31729: [GVNHoist] Don't hoist unsafe scalars at -Oz

[Aditya Kumar via Phabricator via llvm-commits]

hiraditya added a comment.

In https://reviews.llvm.org/D29092#685430, @dberlin wrote:

> In https://reviews.llvm.org/D29092#685418, @hiraditya wrote:
>
> > In https://reviews.llvm.org/D29092#685307, @dberlin wrote:
> >
> > > In https://reviews.llvm.org/D29092#685273, @mehdi_amini wrote:
> > >
> > > > In https://reviews.llvm.org/D29092#685265, @dberlin wrote:
> > > >
> > > > > (Though i quibble with this part:
> > > > >
> > > > >   // The called function could have undefined behavior or
> > > > >   // side-effects, even if marked readnone nounwind.
> > > > >   
> > > > >
> > > > > I'm not sure what we think those side effects are, but we should be modeling them or ignoring them.
> > > >
> > > >
> > > > Divide by zero?
> > > >
> > > > The only current solution we have for this now is to be conservative with respect to calls. There is a new attribute implemented here https://reviews.llvm.org/D20116 that will help making it "less conservative", by allowing to speculate if there can't be UB whatever arguments are passed to the function. 
> > > >  This wouldn't capture this for instance:
> > > >
> > > >   // can be speculated if we know that the argument is never 0
> > > >   int inverse(int n) { return 1/n; }
> > > >
> > > >
> > > > From your GCC experience, do you have a suggestion how to address/model this?
> > >
> > >
> > > First:
> > >
> > >   int __attribute__((noinline))   inverse(int n) { return 1/n; }
> > >    int main(int argc)
> > >    {
> > >  
> > >     int a = 0;
> > >     for (int i = 0; i < 5; ++i)
> > >         a+= inverse(argc);
> > >   return 0
> > >   }
> > >  
> > >
> > >
> > > Is optimized to return 0 at all opt levels (ie it does not consider it enough of a side effect to keep it alive)
> > >  (it's the same whether it's inlined or not)
> > >  If you change it to return a, you get:
> > >
> > >   main (int argc)
> > >    {
> > >      int i;
> > >      int a;
> > >      int _16;
> > >      _16 = inverse (argc_5(D));
> > >      # a_14 = PHI <a_7(3), 0(2)>
> > >      # i_15 = PHI <i_8(3), 0(2)>
> > >      a_7 = a_14 + _16;
> > >      i_8 = i_15 + 1;
> > >      if (i_8 != 5)
> > >        goto <bb 3>;
> > >      else
> > >        goto <bb 4>;
> > >      return a_7;
> > >  
> > >    }
> > >  
> > >
> > >
> > > Note the hoist.
> > >
> > > It then unrolls the loop
> > >  resulting in:
> > >
> > >   main (int argc)
> > >    {
> > >      int a;
> > >      int _16;
> > >  
> > >      _16 = inverse (argc_5(D));
> > >      a_2 = _16 * 5;
> > >      return a_2;
> > >  
> > >    }
> > >  
> > >  
> > >
> > >
> > > If you change it to something it can't prove always executes, it still hoists it to the pre-header, just not above loop condition (obviously).
> > >  So i guess my answer is: We are being too conservative here.
> >
> >
> > Since gcc's gvn hoist works in tandem with gvn-pre, the hoist of a call which occurs only on one branch may have been done by gvn-pre. IIUC gcc's gvn-hoist only hoists instructions which are anticipable (IN) but not available (OUT), that too with basic blocks with > 1 successors (ref: gcc/tree-ssa-pre.c:3499). It is unlikely gvn-hoist is hoisting the call in this example. Another limitation of gcc's gvn-hoist is that it only hoists when at least one immediate successor has the instruction in avail_out. So even if an expression is very busy it may not get hoisted. I understand that this allows them to
> >  not have any cost model for hoisting too far but code-size wise there will be missed opportunities.
>
>
> FWIW, as i mentioned offline, these are trivially fixable without a complex algorithm.
>  They have simply chosen not to because it has not been worth it, AFAIK.
>
> > I agree the design of llvm's gvn-hoist  is not very clean mostly because it went multiple iterations,
>
> Gotta be honest:
>  At some point, you have to take the code that was a prototype, learn from it, and then write a clean version that is sane.


I can do that in a separate patch. This patch related to fixing a correctness bug (at -Os, -Oz). https://bugs.llvm.org//show_bug.cgi?id=31729

> NewGVN went through a ton of iterations and ugly ass code, as chandler can tell you. At some point, we just sat down, said "okay, this is probably a *design* that works, now how do we make it look at least mildly sane".
> 
>> your ideas helped very much to write an optimistic algorithm and incorporate Memory SSA.




https://reviews.llvm.org/D29092