Re: [PATCH][AArch64] Use conditional negate for abs expansion

[James Greenhalgh <james dot greenhalgh at arm dot com>]

On Mon, Apr 27, 2015 at 05:57:26PM +0100, Wilco Dijkstra wrote:
> > James Greenhalgh wrote:
> > On Mon, Apr 27, 2015 at 02:42:36PM +0100, Wilco Dijkstra wrote:
> > > > -----Original Message-----
> > > > From: Wilco Dijkstra [mailto:wdijkstr@arm.com]
> > > > Sent: 03 March 2015 16:19
> > > > To: GCC Patches
> > > > Subject: [PATCH][AArch64] Use conditional negate for abs expansion
> > > >
> > > > Expand abs into a compare and conditional negate. This is the most obvious expansion,
> > enables
> > > > merging of the comparison into ALU instructions and is faster on all implementations.
> > > > Bootstrapped & regression tested.
> > > >
> > > > int f(int x) { return abs (x + 1); }
> > > >
> > > > Before:
> > > >         add     w0, w0, 1
> > > >         sxtw    x0, w0
> > > >         eor     x1, x0, x0, asr 63
> > > >         sub     x1, x1, x0, asr 63
> > > >         mov     x0, x1
> > > >         ret
> > > >
> > > > After:
> > > >         adds    w0, w0, 1
> > > >         csneg   w0, w0, w0, pl
> > > >         ret
> > > >
> > > > ChangeLog:
> > > >
> > > > 2015-03-03  Wilco Dijkstra  <wdijkstr@arm.com>
> > > >
> > > > 	* gcc/config/aarch64/aarch64.md (absdi2): optimize abs expansion.
> > > > 	(csneg3<mode>_insn): enable expansion of pattern.
> > > > 	* gcc/testsuite/gcc.target/aarch64/abs_1.c (abs64): update test
> > > > 	for new abs expansion.  (abs64_in_dreg): likewise.
> > 
> > 
> > This looks like it breaks support for abs in a D register (for example
> > at the end of a loop, or extracted from Neon Intrinsics, etc).
> > 
> > e.g. (totally contrived...)
> > 
> >   int64x1_t
> >   abs_max (int64x2_t x, int64_t *wb)
> >   {
> >     int64_t y = vgetq_lane_s64 (x, 0);
> >     if (y < 0)
> >       y = -y;
> >     return vdup_n_s64 (y);
> >   }
> > 
> > Which currently generates:
> > 
> >   abs_max:
> >           abs     d0, d0
> >           ret
> > 
> > I suppose we don't need to worry too much about that (and the current
> > implementation doesn't seem to catch it reliably anyway), but it would be
> > good if we could keep the support - even if it is rarely used.
> 
> Well it may be possible to write a peephole for this rare case, but I hope
> people would use a vabs if that's what they wanted...

OK, I've seen some of the pain of our current abs expansion code, so I
suppose that we want to get the common case right first, rather
than be too concerned about contrived corner cases.

This patch is OK.

Thanks,
James