[v3,06/15] arm: Fix mve_vmvnq_n_<supf><mode> argument mode

  The vmvnq_n* intrinsics and have [u]int[16|32]_t arguments, so use
<V_elem> iterator instead of HI in mve_vmvnq_n_<supf><mode>.

2022-01-13  Christophe Lyon  <christophe.lyon@foss.st.com>

	gcc/
	* config/arm/mve.md (mve_vmvnq_n_<supf><mode>): Use V_elem mode
	for operand 1.
  

On 13/01/2022 14:56, Christophe Lyon via Gcc-patches wrote:
> The vmvnq_n* intrinsics and have [u]int[16|32]_t arguments, so use
> <V_elem> iterator instead of HI in mve_vmvnq_n_<supf><mode>.
>
> 2022-01-13  Christophe Lyon  <christophe.lyon@foss.st.com>
>
> 	gcc/
> 	* config/arm/mve.md (mve_vmvnq_n_<supf><mode>): Use V_elem mode
> 	for operand 1.
>
> diff --git a/gcc/config/arm/mve.md b/gcc/config/arm/mve.md
> index 171dd384133..5c3b34dce3a 100644
> --- a/gcc/config/arm/mve.md
> +++ b/gcc/config/arm/mve.md
> @@ -617,7 +617,7 @@ (define_insn "mve_vcvtaq_<supf><mode>"
>   (define_insn "mve_vmvnq_n_<supf><mode>"
>     [
>      (set (match_operand:MVE_5 0 "s_register_operand" "=w")
> -	(unspec:MVE_5 [(match_operand:HI 1 "immediate_operand" "i")]
> +	(unspec:MVE_5 [(match_operand:<V_elem> 1 "immediate_operand" "i")]
>   	 VMVNQ_N))
>     ]
>     "TARGET_HAVE_MVE"

While fixing this it might be good to fix the constraint and predicate 
inspired by "DL" and "neon_inv_logic_op2" respectively. This would avoid 
the compiler generating wrong assembly, and instead it would probably 
lead to the compiler using a load literal.

I kind of think it would be better to have the intrinsic refuse the 
immediate altogether, but it seems for NEON we also use the load literal 
approach.
  

On Wed, Jan 19, 2022 at 8:03 PM Andre Vieira (lists) via Gcc-patches <
gcc-patches@gcc.gnu.org> wrote:

>
> On 13/01/2022 14:56, Christophe Lyon via Gcc-patches wrote:
> > The vmvnq_n* intrinsics and have [u]int[16|32]_t arguments, so use
> > <V_elem> iterator instead of HI in mve_vmvnq_n_<supf><mode>.
> >
> > 2022-01-13  Christophe Lyon  <christophe.lyon@foss.st.com>
> >
> >       gcc/
> >       * config/arm/mve.md (mve_vmvnq_n_<supf><mode>): Use V_elem mode
> >       for operand 1.
> >
> > diff --git a/gcc/config/arm/mve.md b/gcc/config/arm/mve.md
> > index 171dd384133..5c3b34dce3a 100644
> > --- a/gcc/config/arm/mve.md
> > +++ b/gcc/config/arm/mve.md
> > @@ -617,7 +617,7 @@ (define_insn "mve_vcvtaq_<supf><mode>"
> >   (define_insn "mve_vmvnq_n_<supf><mode>"
> >     [
> >      (set (match_operand:MVE_5 0 "s_register_operand" "=w")
> > -     (unspec:MVE_5 [(match_operand:HI 1 "immediate_operand" "i")]
> > +     (unspec:MVE_5 [(match_operand:<V_elem> 1 "immediate_operand" "i")]
> >        VMVNQ_N))
> >     ]
> >     "TARGET_HAVE_MVE"
>
> While fixing this it might be good to fix the constraint and predicate
> inspired by "DL" and "neon_inv_logic_op2" respectively. This would avoid
> the compiler generating wrong assembly, and instead it would probably
> lead to the compiler using a load literal.
>
> I kind of think it would be better to have the intrinsic refuse the
> immediate altogether, but it seems for NEON we also use the load literal
> approach.
>
>
Ha, I thought that patch had been approved at v2 too:
https://gcc.gnu.org/pipermail/gcc-patches/2021-October/581344.html
  

On 20/01/2022 09:23, Christophe Lyon wrote:
>
>
> On Wed, Jan 19, 2022 at 8:03 PM Andre Vieira (lists) via Gcc-patches 
> <gcc-patches@gcc.gnu.org> wrote:
>
>
>     On 13/01/2022 14:56, Christophe Lyon via Gcc-patches wrote:
>     > The vmvnq_n* intrinsics and have [u]int[16|32]_t arguments, so use
>     > <V_elem> iterator instead of HI in mve_vmvnq_n_<supf><mode>.
>     >
>     > 2022-01-13  Christophe Lyon  <christophe.lyon@foss.st.com>
>     >
>     >       gcc/
>     >       * config/arm/mve.md (mve_vmvnq_n_<supf><mode>): Use V_elem
>     mode
>     >       for operand 1.
>     >
>     > diff --git a/gcc/config/arm/mve.md b/gcc/config/arm/mve.md
>     > index 171dd384133..5c3b34dce3a 100644
>     > --- a/gcc/config/arm/mve.md
>     > +++ b/gcc/config/arm/mve.md
>     > @@ -617,7 +617,7 @@ (define_insn "mve_vcvtaq_<supf><mode>"
>     >   (define_insn "mve_vmvnq_n_<supf><mode>"
>     >     [
>     >      (set (match_operand:MVE_5 0 "s_register_operand" "=w")
>     > -     (unspec:MVE_5 [(match_operand:HI 1 "immediate_operand" "i")]
>     > +     (unspec:MVE_5 [(match_operand:<V_elem> 1
>     "immediate_operand" "i")]
>     >        VMVNQ_N))
>     >     ]
>     >     "TARGET_HAVE_MVE"
>
>     While fixing this it might be good to fix the constraint and
>     predicate
>     inspired by "DL" and "neon_inv_logic_op2" respectively. This would
>     avoid
>     the compiler generating wrong assembly, and instead it would probably
>     lead to the compiler using a load literal.
>
>     I kind of think it would be better to have the intrinsic refuse the
>     immediate altogether, but it seems for NEON we also use the load
>     literal
>     approach.
>
>
> Ha, I thought that patch had been approved at v2 too: 
> https://gcc.gnu.org/pipermail/gcc-patches/2021-October/581344.html
>
Yeah sorry I had not looked at the previous version of these series!

I can put together a follow-up for this then.
  

On Thu, Jan 20, 2022 at 10:38 AM Andre Simoes Dias Vieira <
andre.simoesdiasvieira@arm.com> wrote:

>
> On 20/01/2022 09:23, Christophe Lyon wrote:
>
>
>
> On Wed, Jan 19, 2022 at 8:03 PM Andre Vieira (lists) via Gcc-patches <
> gcc-patches@gcc.gnu.org> wrote:
>
>>
>> On 13/01/2022 14:56, Christophe Lyon via Gcc-patches wrote:
>> > The vmvnq_n* intrinsics and have [u]int[16|32]_t arguments, so use
>> > <V_elem> iterator instead of HI in mve_vmvnq_n_<supf><mode>.
>> >
>> > 2022-01-13  Christophe Lyon  <christophe.lyon@foss.st.com>
>> >
>> >       gcc/
>> >       * config/arm/mve.md (mve_vmvnq_n_<supf><mode>): Use V_elem mode
>> >       for operand 1.
>> >
>> > diff --git a/gcc/config/arm/mve.md b/gcc/config/arm/mve.md
>> > index 171dd384133..5c3b34dce3a 100644
>> > --- a/gcc/config/arm/mve.md
>> > +++ b/gcc/config/arm/mve.md
>> > @@ -617,7 +617,7 @@ (define_insn "mve_vcvtaq_<supf><mode>"
>> >   (define_insn "mve_vmvnq_n_<supf><mode>"
>> >     [
>> >      (set (match_operand:MVE_5 0 "s_register_operand" "=w")
>> > -     (unspec:MVE_5 [(match_operand:HI 1 "immediate_operand" "i")]
>> > +     (unspec:MVE_5 [(match_operand:<V_elem> 1 "immediate_operand" "i")]
>> >        VMVNQ_N))
>> >     ]
>> >     "TARGET_HAVE_MVE"
>>
>> While fixing this it might be good to fix the constraint and predicate
>> inspired by "DL" and "neon_inv_logic_op2" respectively. This would avoid
>> the compiler generating wrong assembly, and instead it would probably
>> lead to the compiler using a load literal.
>>
>> I kind of think it would be better to have the intrinsic refuse the
>> immediate altogether, but it seems for NEON we also use the load literal
>> approach.
>>
>>
> Ha, I thought that patch had been approved at v2 too:
> https://gcc.gnu.org/pipermail/gcc-patches/2021-October/581344.html
>
> Yeah sorry I had not looked at the previous version of these series!
>
> I can put together a follow-up for this then.
>

No problem, thanks!
  

"Andre Vieira (lists)" <andre.simoesdiasvieira@arm.com> writes:
> On 13/01/2022 14:56, Christophe Lyon via Gcc-patches wrote:
>> The vmvnq_n* intrinsics and have [u]int[16|32]_t arguments, so use
>> <V_elem> iterator instead of HI in mve_vmvnq_n_<supf><mode>.
>>
>> 2022-01-13  Christophe Lyon  <christophe.lyon@foss.st.com>
>>
>> 	gcc/
>> 	* config/arm/mve.md (mve_vmvnq_n_<supf><mode>): Use V_elem mode
>> 	for operand 1.
>>
>> diff --git a/gcc/config/arm/mve.md b/gcc/config/arm/mve.md
>> index 171dd384133..5c3b34dce3a 100644
>> --- a/gcc/config/arm/mve.md
>> +++ b/gcc/config/arm/mve.md
>> @@ -617,7 +617,7 @@ (define_insn "mve_vcvtaq_<supf><mode>"
>>   (define_insn "mve_vmvnq_n_<supf><mode>"
>>     [
>>      (set (match_operand:MVE_5 0 "s_register_operand" "=w")
>> -	(unspec:MVE_5 [(match_operand:HI 1 "immediate_operand" "i")]
>> +	(unspec:MVE_5 [(match_operand:<V_elem> 1 "immediate_operand" "i")]
>>   	 VMVNQ_N))
>>     ]
>>     "TARGET_HAVE_MVE"
>
> While fixing this it might be good to fix the constraint and predicate 
> inspired by "DL" and "neon_inv_logic_op2" respectively. This would avoid 
> the compiler generating wrong assembly, and instead it would probably 
> lead to the compiler using a load literal.

FWIW: for cases like this, I think it's better to define a predicate
only (not a constraint).  By design, the only time that constraints
are used independently of predicates is during RA, and there's nothing
that RA can/should do for immediate operands.

Thanks,
Richard
  

On 20/01/2022 10:45, Richard Sandiford wrote:
> "Andre Vieira (lists)" <andre.simoesdiasvieira@arm.com> writes:
>> On 13/01/2022 14:56, Christophe Lyon via Gcc-patches wrote:
>>> The vmvnq_n* intrinsics and have [u]int[16|32]_t arguments, so use
>>> <V_elem> iterator instead of HI in mve_vmvnq_n_<supf><mode>.
>>>
>>> 2022-01-13  Christophe Lyon  <christophe.lyon@foss.st.com>
>>>
>>> 	gcc/
>>> 	* config/arm/mve.md (mve_vmvnq_n_<supf><mode>): Use V_elem mode
>>> 	for operand 1.
>>>
>>> diff --git a/gcc/config/arm/mve.md b/gcc/config/arm/mve.md
>>> index 171dd384133..5c3b34dce3a 100644
>>> --- a/gcc/config/arm/mve.md
>>> +++ b/gcc/config/arm/mve.md
>>> @@ -617,7 +617,7 @@ (define_insn "mve_vcvtaq_<supf><mode>"
>>>    (define_insn "mve_vmvnq_n_<supf><mode>"
>>>      [
>>>       (set (match_operand:MVE_5 0 "s_register_operand" "=w")
>>> -	(unspec:MVE_5 [(match_operand:HI 1 "immediate_operand" "i")]
>>> +	(unspec:MVE_5 [(match_operand:<V_elem> 1 "immediate_operand" "i")]
>>>    	 VMVNQ_N))
>>>      ]
>>>      "TARGET_HAVE_MVE"
>> While fixing this it might be good to fix the constraint and predicate
>> inspired by "DL" and "neon_inv_logic_op2" respectively. This would avoid
>> the compiler generating wrong assembly, and instead it would probably
>> lead to the compiler using a load literal.
> FWIW: for cases like this, I think it's better to define a predicate
> only (not a constraint).  By design, the only time that constraints
> are used independently of predicates is during RA, and there's nothing
> that RA can/should do for immediate operands.
>
> Thanks,
> Richard
Yeah, if I use a predicate it doesn't like the fact that we are passing 
an argument 'imm' rather than actual immediate. To use a constraint like 
DL I'd also need to change the builtin to take a vector of immediates, 
since we can't use immediates as they don't have a mode and the 
constraint needs to be able to know what mode we are using.

This will have to wait...