Re: [PATCH] fold a * (a > 0 ? 1 : -1) to abs(a) and related optimizations

[Marc Glisse <marc.glisse@inria.fr>]

On Fri, 23 Jun 2017, Andrew Pinski wrote:

> Hi,
>  I saw this on llvm's review site (https://reviews.llvm.org/D34579)
> and I thought why not add it to GCC.  I expanded more than what was
> done on the LLVM patch.
>
> I added the following optimizations:
> Transform X * (X > 0 ? 1 : -1) into ABS(X).
> Transform X * (X >= 0 ? 1 : -1) into ABS(X).
> Transform X * (X > 0.0 ? 1.0 : -1.0) into ABS(X).
> Transform X * (X >= 0.0 ? 1.0 : -1.0) into ABS(X).
> Transform X * (X > 0 ? -1 : 1) into -ABS(X).
> Transform X * (X >= 0 ? -1 : 1) into -ABS(X).
> Transform X * (X > 0.0 ? -1.0 : 1.0) into -ABS(X).
> Transform X * (X >= 0.0 ? -1.0 : 1.0) into -ABS(X).
> Transform X * (X < 0 ? 1 : -1) into -ABS(X).
> Transform X * (X <= 0 ? 1 : -1) into -ABS(X).
> Transform X * (X < 0.0 ? 1.0 : -1.0) into -ABS(X).
> Transform X * (X <= 0.0 ? 1.0 : -1.0) into -ABS(X).
> Transform X * (X < 0 ? -1 : 1) into ABS(X).
> Transform X * (X <= 0 ? -1 : 1) into ABS(X).
> Transform X * (X < 0.0 ? -1.0 : 1.0) into ABS(X).
> Transform X * (X <= 0.0 ? -1.0 : 1.0) into ABS(X).
>
> The floating points ones only happen when not honoring SNANS and not
> honoring signed zeros.

Some random comments (not a review):

* if X is NaN, we may get a qNaN with the wrong sign bit. We probably 
don't care much though...

* I am surprised (X<0.?-1.:1.) and copysign(1., X) remain different for 
the whole optimization pipeline with -ffast-math. X*copysign(1., X) is 
another candidate to become fabs(X).

* Whenever you get -ABS(X) for integers, what about the case where X is 
INT_MIN?

* I guess we can't get there with an unsigned type because X>0 would have 
become X!=0 .

* I wonder if we could use something like

(for cmp (gt ge lt le)
      outp (convert convert negate negate)
      outn (negate negate convert convert)
[...]
   (outp (abs @0))

to reduce duplication or if that would be less readable.

* Some of the cases are handled by PRE turning

   # iftmp.0_1 = PHI <1.0e+0(5), -1.0e+0(3)>
   _3 = iftmp.0_1 * a_2(D);

into

   _5 = -a_2(D);
[...]
   # iftmp.0_1 = PHI <1.0e+0(2), -1.0e+0(3)>
   # prephitmp_6 = PHI <a_2(D)(2), _5(3)>

which phiopt3 can handle (quite late).

* With cond, this currently (?) only affects generic, so I am not sure it 
will hit very often... But it will be there if someone later writes a 
match.pd->phiopt generator ;-)

-- 
Marc Glisse