| Message ID | 20d007c8-f703-8fac-6a13-1643af31adc6@suse.com |
|---|---|
| State | New |
| Headers | show |
| Series | x86: {,v}psadbw have commutative source operands | expand |
On Fri, May 27, 2022 at 10:13 AM Jan Beulich <jbeulich@suse.com> wrote: > > Like noticed for gas as well (binutils-gdb commit c8cad9d389b7), the > "absolute difference" aspect of the insns makes their source operands > commutative. You will need to expand via ix86_fixup_binary_operands_no_copy, use register_mmxmem_operand on both input operands and use ix86_binary_operator insn constraint. Please see many examples w/ commutative operands throughout .md files. Uros. > gcc/ > 2022-05-XX Jan Beulich <jbeulich@suse.com> > > * config/i386/mmx.md (mmx_psadbw): Mark as commutative. > * config/i386/sse.md (<sse2_avx2>_psadbw): Likewise. > > --- a/gcc/config/i386/mmx.md > +++ b/gcc/config/i386/mmx.md > @@ -4407,7 +4407,7 @@ > > (define_insn "mmx_psadbw" > [(set (match_operand:V1DI 0 "register_operand" "=y,x,Yw") > - (unspec:V1DI [(match_operand:V8QI 1 "register_operand" "0,0,Yw") > + (unspec:V1DI [(match_operand:V8QI 1 "register_operand" "%0,0,Yw") > (match_operand:V8QI 2 "register_mmxmem_operand" "ym,x,Yw")] > UNSPEC_PSADBW))] > "(TARGET_MMX || TARGET_MMX_WITH_SSE) > --- a/gcc/config/i386/sse.md > +++ b/gcc/config/i386/sse.md > @@ -19983,7 +19983,7 @@ > (define_insn "<sse2_avx2>_psadbw" > [(set (match_operand:VI8_AVX2_AVX512BW 0 "register_operand" "=x,YW") > (unspec:VI8_AVX2_AVX512BW > - [(match_operand:<ssebytemode> 1 "register_operand" "0,YW") > + [(match_operand:<ssebytemode> 1 "register_operand" "%0,YW") > (match_operand:<ssebytemode> 2 "vector_operand" "xBm,YWm")] > UNSPEC_PSADBW))] > "TARGET_SSE2" >
On 27.05.2022 11:05, Uros Bizjak wrote: > On Fri, May 27, 2022 at 10:13 AM Jan Beulich <jbeulich@suse.com> wrote: >> >> Like noticed for gas as well (binutils-gdb commit c8cad9d389b7), the >> "absolute difference" aspect of the insns makes their source operands >> commutative. > > You will need to expand via ix86_fixup_binary_operands_no_copy, use > register_mmxmem_operand on both input operands and use > ix86_binary_operator insn constraint. Please see many examples w/ > commutative operands throughout .md files. Hmm, yes, I see. As to the use of ix86_binary_operator_ok(): In particular in sse.md I see many uses of ix86_fixup_binary_operands_no_copy() in expanders where the corresponding insns don't use ix86_binary_operator_ok(), e.g. the immediately preceding uavg. Is there a(n) (anti-)pattern? My simplistic initial version was based on observations while putting together the inverse change for vgf2p8affine{,inv}qb_<mode><mask_name> (commit c0569d342ca4), which aren't commutative. Are you suggesting that the remaining (for indeed being commutative) vgf2p8mulb_<mode><mask_name> also is incomplete, requiring an expander as well? And maybe the same then in <code>v1ti3 for any_logic:V1TI, avx512bw_umulhrswv32hi3<mask_name>, or <sse4_1>_dp<ssemodesuffix><avxsizesuffix> (and likely a few more)? At least a few pmadd* appear to lack commutativity marking altogether. Jan >> --- a/gcc/config/i386/mmx.md >> +++ b/gcc/config/i386/mmx.md >> @@ -4407,7 +4407,7 @@ >> >> (define_insn "mmx_psadbw" >> [(set (match_operand:V1DI 0 "register_operand" "=y,x,Yw") >> - (unspec:V1DI [(match_operand:V8QI 1 "register_operand" "0,0,Yw") >> + (unspec:V1DI [(match_operand:V8QI 1 "register_operand" "%0,0,Yw") >> (match_operand:V8QI 2 "register_mmxmem_operand" "ym,x,Yw")] >> UNSPEC_PSADBW))] >> "(TARGET_MMX || TARGET_MMX_WITH_SSE) >> --- a/gcc/config/i386/sse.md >> +++ b/gcc/config/i386/sse.md >> @@ -19983,7 +19983,7 @@ >> (define_insn "<sse2_avx2>_psadbw" >> [(set (match_operand:VI8_AVX2_AVX512BW 0 "register_operand" "=x,YW") >> (unspec:VI8_AVX2_AVX512BW >> - [(match_operand:<ssebytemode> 1 "register_operand" "0,YW") >> + [(match_operand:<ssebytemode> 1 "register_operand" "%0,YW") >> (match_operand:<ssebytemode> 2 "vector_operand" "xBm,YWm")] >> UNSPEC_PSADBW))] >> "TARGET_SSE2" >> >
On Mon, May 30, 2022 at 9:59 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 27.05.2022 11:05, Uros Bizjak wrote: > > On Fri, May 27, 2022 at 10:13 AM Jan Beulich <jbeulich@suse.com> wrote: > >> > >> Like noticed for gas as well (binutils-gdb commit c8cad9d389b7), the > >> "absolute difference" aspect of the insns makes their source operands > >> commutative. > > > > You will need to expand via ix86_fixup_binary_operands_no_copy, use > > register_mmxmem_operand on both input operands and use > > ix86_binary_operator insn constraint. Please see many examples w/ > > commutative operands throughout .md files. > > Hmm, yes, I see. As to the use of ix86_binary_operator_ok(): In > particular in sse.md I see many uses of > ix86_fixup_binary_operands_no_copy() in expanders where the > corresponding insns don't use ix86_binary_operator_ok(), e.g. the > immediately preceding uavg. Is there a(n) (anti-)pattern? ix86_fixup_binary_operands was historically used with destructive two-operand instructions (where one input operand is tied with output operand). It fixed up expansion to help register allocator a bit, and brought expansion to the form of two-operand instruction, especially with memory and immediate operands. Please note that nowadays RA can fix up the operands by itself, but it is still beneficial to have e.g. memory operation in the right place from the beginning. > My simplistic initial version was based on observations while > putting together the inverse change for > vgf2p8affine{,inv}qb_<mode><mask_name> (commit c0569d342ca4), which > aren't commutative. Are you suggesting that the remaining (for indeed > being commutative) vgf2p8mulb_<mode><mask_name> also is incomplete, > requiring an expander as well? And maybe the same then in > <code>v1ti3 for any_logic:V1TI, avx512bw_umulhrswv32hi3<mask_name>, > or <sse4_1>_dp<ssemodesuffix><avxsizesuffix> (and likely a few more)? > > At least a few pmadd* appear to lack commutativity marking altogether. Yes, sse.md could use some TLC in this area. I remember doing a review of these patterns in i386.md a while ago. Uros. > Jan > > >> --- a/gcc/config/i386/mmx.md > >> +++ b/gcc/config/i386/mmx.md > >> @@ -4407,7 +4407,7 @@ > >> > >> (define_insn "mmx_psadbw" > >> [(set (match_operand:V1DI 0 "register_operand" "=y,x,Yw") > >> - (unspec:V1DI [(match_operand:V8QI 1 "register_operand" "0,0,Yw") > >> + (unspec:V1DI [(match_operand:V8QI 1 "register_operand" "%0,0,Yw") > >> (match_operand:V8QI 2 "register_mmxmem_operand" "ym,x,Yw")] > >> UNSPEC_PSADBW))] > >> "(TARGET_MMX || TARGET_MMX_WITH_SSE) > >> --- a/gcc/config/i386/sse.md > >> +++ b/gcc/config/i386/sse.md > >> @@ -19983,7 +19983,7 @@ > >> (define_insn "<sse2_avx2>_psadbw" > >> [(set (match_operand:VI8_AVX2_AVX512BW 0 "register_operand" "=x,YW") > >> (unspec:VI8_AVX2_AVX512BW > >> - [(match_operand:<ssebytemode> 1 "register_operand" "0,YW") > >> + [(match_operand:<ssebytemode> 1 "register_operand" "%0,YW") > >> (match_operand:<ssebytemode> 2 "vector_operand" "xBm,YWm")] > >> UNSPEC_PSADBW))] > >> "TARGET_SSE2" > >> > > >
--- a/gcc/config/i386/mmx.md +++ b/gcc/config/i386/mmx.md @@ -4407,7 +4407,7 @@ (define_insn "mmx_psadbw" [(set (match_operand:V1DI 0 "register_operand" "=y,x,Yw") - (unspec:V1DI [(match_operand:V8QI 1 "register_operand" "0,0,Yw") + (unspec:V1DI [(match_operand:V8QI 1 "register_operand" "%0,0,Yw") (match_operand:V8QI 2 "register_mmxmem_operand" "ym,x,Yw")] UNSPEC_PSADBW))] "(TARGET_MMX || TARGET_MMX_WITH_SSE) --- a/gcc/config/i386/sse.md +++ b/gcc/config/i386/sse.md @@ -19983,7 +19983,7 @@ (define_insn "<sse2_avx2>_psadbw" [(set (match_operand:VI8_AVX2_AVX512BW 0 "register_operand" "=x,YW") (unspec:VI8_AVX2_AVX512BW - [(match_operand:<ssebytemode> 1 "register_operand" "0,YW") + [(match_operand:<ssebytemode> 1 "register_operand" "%0,YW") (match_operand:<ssebytemode> 2 "vector_operand" "xBm,YWm")] UNSPEC_PSADBW))] "TARGET_SSE2"