On Mon, Jul 08, 2013 at 04:32:13PM +0100, Zhenqiang Chen wrote:
> On 8 July 2013 20:57, Ramana Radhakrishnan <ramana....@googlemail.com> wrote:
> > Not yet. Why is this not a problem in the LT / UNLT case ?
>
> From the context, after the first switch, only GE/LE/EQ might have
> operands[5] which is not REG (CONST0_RTX). For others including
> LT/UNLT, operands[5] should be REG.
This is true, but looks like an omission. My copy of the ARMARM
has immediate #0 instruction forms for CMLT, CMLE, CMGE, CMGT
and CMEQ. Perhaps it is beyond the scope of your bugfix
(though it was in your original patch?), but this should be
fixed in future so as not to force 0 to registers.
>
> if (!REG_P (operands[5]))
> operands[5] = force_reg (<MODE>mode, operands[5]);
>
> For GE/LE/EQ, we only reverse LE. So only LE has issue.
>
For now, but as above - as soon as this code is fixed to generate
immediate #0 forms, it will be fragile again.
> >> diff --git a/gcc/config/arm/neon.md b/gcc/config/arm/neon.md
> >> index 2761adb..6d9f604 100644
> >> --- a/gcc/config/arm/neon.md
> >> +++ b/gcc/config/arm/neon.md
> >> @@ -1710,6 +1710,9 @@
> >> case LE:
> >> case UNLE:
> >> inverse = 1;
> >> + /* Can not inverse "a LE 0" to "0 GE a". */
> >> + if (operands[5] == CONST0_RTX (<MODE>mode))
> >> + inverse = 0;
> >> /* Fall through. */
> >> case GT:
> >> case UNGT:
Is this really what you mean? Surely now you will have:
inverse = 0
base_comparison = gen_neon_vcgt
Thus in the next switch you will call:
emit_insn (gen_neon_vcgt (mask, operands[4], operands[5], magic_rtx));
Which looks wrong. Would you not also have to set swap_bsl_operands
to get back to the correct semantics?
> >> diff --git a/gcc/testsuite/gcc.target/arm/lp1189445.c
> >> b/gcc/testsuite/gcc.target/arm/lp1189445.c
> >> new file mode 100644
> >> index 0000000..8ce4b97
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.target/arm/lp1189445.c
> >> @@ -0,0 +1,16 @@
> >> +/* { dg-do compile } */
> >> +/* { dg-options "-O3 -mfpu=neon -mcpu=cortex-a9 -mthumb
> >> -mfloat-abi=hard -S" } */
> >> +
> >> +int id;
> >> +int
> >> +test (const long int *data)
> >> +{
> >> + int i, retval;
> >> + retval = id;
> >> + for (i = 0; i < id; i++)
> >> + {
> >> + retval &= (data[i] <= 0);
> >> + }
> >> +
> >> + return (retval);
> >> +}
>
This testcase is not much use. It may well compile, but won't catch
the wrong instruction generation issue I pointed out above.
I much prefer your original patch, with a more rigorous testcase.
Thanks,
James
