Message ID | 20220113145645.4077141-7-christophe.lyon@foss.st.com |
---|---|
State | New |
Headers | show |
Series | ARM/MVE use vectors of boolean for predicates | expand |
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...
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"