[Patch AArch64] Stop generating BSL for simple integer code

[James Greenhalgh]

On Thu, Jul 27, 2017 at 06:49:01PM +0100, James Greenhalgh wrote:
>
> On Mon, Jun 12, 2017 at 02:44:40PM +0100, James Greenhalgh wrote:
> > [Sorry for the re-send. I spotted that the attributes were not right for the
> >  new pattern I was adding. The change between this and the first version was:
> >
> >   +  [(set_attr "type" "neon_bsl,neon_bsl,neon_bsl,multiple")
> >   +   (set_attr "length" "4,4,4,12")]
> > ]
> >
> > ---
> >
> > Hi,
> >
> > In this testcase, all argument registers and the return register
> > will be general purpose registers:
> >
> >   long long
> >   foo (long long a, long long b, long long c)
> >   {
> >     return ((a ^ b) & c) ^ b;
> >   }
> >
> > However, due to the implementation of aarch64_simd_bsl<mode>_internal
> > we'll match that pattern and emit a BSL, necessitating moving all those
> > arguments and results to the Advanced SIMD registers:
> >
> > 	fmov	d2, x0
> > 	fmov	d0, x2
> > 	fmov	d1, x1
> > 	bsl	v0.8b, v2.8b, v1.8b
> > 	fmov	x0, d0
> >
> > To fix this, we turn aarch64_simd_bsldi_internal in to an insn_and_split that
> > knows to split back to integer operations if the register allocation
> > falls that way.
> >
> > We could have used an unspec, but then we lose some of the nice
> > simplifications that can be made from explicitly spelling out the semantics
> > of BSL.
>
> Off list, Richard and I found considerable issues with this patch. From
> idioms failing to match in the testcase, to subtle register allocation bugs,
> to potential for suboptimal code generation.
>
> That's not good!
>
> This spin of the patch corrects those issues by adding a second split
> pattern, allocating a temporary register if we're permitted to, or
> properly using tied output operands if we're not, and by generally playing
> things a bit safer around the possibility of register overlaps.
>
> The testcase is expanded to an execute test, hopefully giving a little
> more assurance that we're doing the right thing - now testing the BSL idiom
> generation in both general-purpose and floating-point registers and comparing
> the results. Hopefully we're now a bit more robust!
>
> Bootstrapped and tested on aarch64-none-linux-gnu and cross-tested on
> aarch64-none-elf with no issues.
>
> OK for trunk?

I'd like to ping this patch for review (I would like the explicit review as
an earlier revision had bugs), attached is a rebase of this patch on trunk.

OK? If so, please commit it on my behalf as I will be out of office for a
few weeks for some time off.

Thanks,
James

---
gcc/

2017-10-04  James Greenhalgh  <james.greenhalgh@arm.com>

	* config/aarch64/aarch64-simd.md
	(aarch64_simd_bsl<mode>_internal): Remove DImode.
	(*aarch64_simd_bsl<mode>_alt): Likewise.
	(aarch64_simd_bsldi_internal): New.
	(aarch64_simd_bsldi_alt): Likewise.

gcc/testsuite/

2017-10-04  James Greenhalgh  <james.greenhalgh@arm.com>

	* gcc.target/aarch64/bsl-idiom.c: New.
	* gcc.target/aarch64/copysign-bsl.c: New.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Re-Patch-AArch64-Stop-generating-BSL-for-simple-inte.patch
Type: text/x-patch
Size: 8551 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20171004/89cb4625/attachment.bin>