Re: [PATCH v3 06/15] arm: Fix mve_vmvnq_n_ argument mode
On 20/01/2022 10:45, Richard Sandiford wrote: "Andre Vieira (lists)" 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 iterator instead of HI in mve_vmvnq_n_. 2022-01-13 Christophe Lyon gcc/ * config/arm/mve.md (mve_vmvnq_n_): 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_" (define_insn "mve_vmvnq_n_" [ (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: 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...
Re: [PATCH v3 06/15] arm: Fix mve_vmvnq_n_ argument mode
"Andre Vieira (lists)" 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 >> iterator instead of HI in mve_vmvnq_n_. >> >> 2022-01-13 Christophe Lyon >> >> gcc/ >> * config/arm/mve.md (mve_vmvnq_n_): 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_" >> (define_insn "mve_vmvnq_n_" >> [ >> (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: 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
Re: [PATCH v3 06/15] arm: Fix mve_vmvnq_n_ argument mode
On Thu, Jan 20, 2022 at 10:38 AM Andre Simoes Dias Vieira < andre.simoesdiasvie...@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 >> > iterator instead of HI in mve_vmvnq_n_. >> > >> > 2022-01-13 Christophe Lyon >> > >> > gcc/ >> > * config/arm/mve.md (mve_vmvnq_n_): 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_" >> > (define_insn "mve_vmvnq_n_" >> > [ >> > (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: 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!
Re: [PATCH v3 06/15] arm: Fix mve_vmvnq_n_ argument mode
On 20/01/2022 09:23, Christophe Lyon wrote: On Wed, Jan 19, 2022 at 8:03 PM Andre Vieira (lists) via Gcc-patches 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 > iterator instead of HI in mve_vmvnq_n_. > > 2022-01-13 Christophe Lyon > > gcc/ > * config/arm/mve.md (mve_vmvnq_n_): 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_" > (define_insn "mve_vmvnq_n_" > [ > (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: 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.
Re: [PATCH v3 06/15] arm: Fix mve_vmvnq_n_ argument mode
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 > > iterator instead of HI in mve_vmvnq_n_. > > > > 2022-01-13 Christophe Lyon > > > > gcc/ > > * config/arm/mve.md (mve_vmvnq_n_): 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_" > > (define_insn "mve_vmvnq_n_" > > [ > > (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: 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
Re: [PATCH v3 06/15] arm: Fix mve_vmvnq_n_ argument mode
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 iterator instead of HI in mve_vmvnq_n_. 2022-01-13 Christophe Lyon gcc/ * config/arm/mve.md (mve_vmvnq_n_): 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_" (define_insn "mve_vmvnq_n_" [ (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: 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.
[PATCH v3 06/15] arm: Fix mve_vmvnq_n_ argument mode
The vmvnq_n* intrinsics and have [u]int[16|32]_t arguments, so use iterator instead of HI in mve_vmvnq_n_. 2022-01-13 Christophe Lyon gcc/ * config/arm/mve.md (mve_vmvnq_n_): 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_" (define_insn "mve_vmvnq_n_" [ (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: 1 "immediate_operand" "i")] VMVNQ_N)) ] "TARGET_HAVE_MVE" -- 2.25.1
