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

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

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.


https://reviews.llvm.org/D29092