ARM/NEON: vld1q_dup_s64 builtin

Hello,

On ARM+Neon, the expansion of vld1q_dup_s64() and vld1q_dup_u64() builtins currently fails to load the second vector element.

Here is a small patch to address this problem:

2012-05-07  Christophe Lyon <christophe.lyon@st.com>

     * gcc/config/arm/neon.md (neon_vld1_dup): Fix vld1q_dup_s64.


OK?

Thanks,

Christophe.

On 9 May 2012 11:18, Christophe Lyon <christophe.lyon@st.com> wrote:
> Hello,
>
> On ARM+Neon, the expansion of vld1q_dup_s64() and vld1q_dup_u64() builtins
> currently fails to load the second vector element.

Thanks for the patch but this is not acceptable as it stands today.
You need to set the length attributes in this case to 8 for the
appropriate alternative at the very least. You also don't mention how
this patch was tested. Alternatively it might be worth splitting the
vld1q_*64 case into a 64 bit load into a (subreg:DI (V2DI reg)  0 )
followed by a subreg to subreg move which should end up having the
same effect . That splitting would allow for better instruction
scheduling. In addition it would be nice to have a testcase in
gcc.target/arm .

As a follow up patch I'd like these patterns merged with the vdup_n
patterns in neon.md (allowing them to grow a memory operand variant)
which should then allow merging of (I think)

scalarval = scalar_load ()
vreg = vdup ( scalarval)

into

vreg = vld1_dup_n ( scalar_address).

Thanks,
Ramana

On 10.05.2012 13:41, Ramana Radhakrishnan wrote:
> On 9 May 2012 11:18, Christophe Lyon<christophe.lyon@st.com>  wrote:
>> Hello,
>>
>> On ARM+Neon, the expansion of vld1q_dup_s64() and vld1q_dup_u64() builtins
>> currently fails to load the second vector element.
> Thanks for the patch but this is not acceptable as it stands today.
> You need to set the length attributes in this case to 8 for the
> appropriate alternative at the very least.
OK I'll look at this.

> You also don't mention how this patch was tested.
I used the testsuite I developed some time ago to test all the Neon builtins, which I posted last year on the qemu mailing-list. With the current GCCs, this bug is the only remaining one I could detect.

>   Alternatively it might be worth splitting the
> vld1q_*64 case into a 64 bit load into a (subreg:DI (V2DI reg)  0 )
> followed by a subreg to subreg move which should end up having the
> same effect . That splitting would allow for better instruction
> scheduling.
Are you aware of examples of similar cases I could use as a model?

>   In addition it would be nice to have a testcase in
> gcc.target/arm .
Well. Prior to sending my patch I did look at that directory, but I supposed that such a test ought to belong to the neon/ subdir where the tests are described as autogenerated. Any doc on how to do that?

Thanks,

Christophe.

On Thu, 10 May 2012 17:31:43 +0200
Christophe Lyon <christophe.lyon@st.com> wrote:

> On 10.05.2012 13:41, Ramana Radhakrishnan wrote:
> > On 9 May 2012 11:18, Christophe Lyon<christophe.lyon@st.com>  wrote:
> >> Hello,
> >>
> >> On ARM+Neon, the expansion of vld1q_dup_s64() and vld1q_dup_u64()
> >> builtins currently fails to load the second vector element.
> > Thanks for the patch but this is not acceptable as it stands today.
> > You need to set the length attributes in this case to 8 for the
> > appropriate alternative at the very least.
> OK I'll look at this.
> 
> > You also don't mention how this patch was tested.
> I used the testsuite I developed some time ago to test all the Neon
> builtins, which I posted last year on the qemu mailing-list. With the
> current GCCs, this bug is the only remaining one I could detect.
> 
> >   Alternatively it might be worth splitting the
> > vld1q_*64 case into a 64 bit load into a (subreg:DI (V2DI reg)  0 )
> > followed by a subreg to subreg move which should end up having the
> > same effect . That splitting would allow for better instruction
> > scheduling.
> Are you aware of examples of similar cases I could use as a model?
> 
> >   In addition it would be nice to have a testcase in
> > gcc.target/arm .
> Well. Prior to sending my patch I did look at that directory, but I
> supposed that such a test ought to belong to the neon/ subdir where
> the tests are described as autogenerated. Any doc on how to do that?

I'd recommend not to autogenerate such a test, FWIW -- the
autogenerated neon tests aren't very good. I think a manually-written
execute test would be better in this case.

If you do try autogenerating tests, look at "Disassembles_as" in
neon.ml, and neon-testgen.ml.

Julian

>
>
>> You also don't mention how this patch was tested.
>
> I used the testsuite I developed some time ago to test all the Neon
> builtins, which I posted last year on the qemu mailing-list. With the
> current GCCs, this bug is the only remaining one I could detect.
>

Fair enough.


>
>>  Alternatively it might be worth splitting the
>> vld1q_*64 case into a 64 bit load into a (subreg:DI (V2DI reg)  0 )
>> followed by a subreg to subreg move which should end up having the
>> same effect . That splitting would allow for better instruction
>> scheduling.
>
> Are you aware of examples of similar cases I could use as a model?

I would change the iterator from VQX to VQ in the pattern above (you
can also simplify the setting of neon_type in that case as well as
change that to be a vec_duplicate as below and get rid of any
lingering definitions of UNSPEC_VLD1_DUP if they exist), define a
separate pattern that expressed this as a define_insn_and_split as
below.

 (define_insn_and_split "neon_vld1_dupv2di"
   [(set (match_operand:V2DI 0 "s_register_operand" "=w")
     (vec_duplicate:V2DI (match_operand:DI 1 "neon_struct_operand" "Um")))]
   "TARGET_NEON"
   "#"
   "&& reload_completed"
   [(const_int 0)]
   {
    rtx tmprtx = gen_lowpart (DImode, operands[0]);
    emit_insn (gen_neon_vld1_dupdi (tmprtx, operands[1]));
    emit_move_insn (gen_highpart (DImode, operands[0]), tmprtx );
    DONE;
    }
(set_attr "length" "8")
(set_attr "neon_type" "<fromearlierpattern">)
)

Do you want to try this and see what you get ?

>
>
>>  In addition it would be nice to have a testcase in
>> gcc.target/arm .
>
> Well. Prior to sending my patch I did look at that directory, but I supposed
> that such a test ought to belong to the neon/ subdir where the tests are
> described as autogenerated. Any doc on how to do that?

 I'd rather have an extra regression test in gcc.target/arm that was a
run time test. for e.g. take a look at gcc.target/arm/neon-vadds64.c .

Ramana

>
> Thanks,
>
> Christophe.
>