Re: [PATCH] x86: {,v}psadbw have commutative source operands
On Mon, May 30, 2022 at 9:59 AM Jan Beulich wrote:
>
> On 27.05.2022 11:05, Uros Bizjak wrote:
> > On Fri, May 27, 2022 at 10:13 AM Jan Beulich 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_ (commit c0569d342ca4), which
> aren't commutative. Are you suggesting that the remaining (for indeed
> being commutative) vgf2p8mulb_ also is incomplete,
> requiring an expander as well? And maybe the same then in
> v1ti3 for any_logic:V1TI, avx512bw_umulhrswv32hi3,
> or _dp (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 "_psadbw"
> >>[(set (match_operand:VI8_AVX2_AVX512BW 0 "register_operand" "=x,YW")
> >> (unspec:VI8_AVX2_AVX512BW
> >> - [(match_operand: 1 "register_operand" "0,YW")
> >> + [(match_operand: 1 "register_operand" "%0,YW")
> >>(match_operand: 2 "vector_operand" "xBm,YWm")]
> >> UNSPEC_PSADBW))]
> >>"TARGET_SSE2"
> >>
> >
>
Re: [PATCH] x86: {,v}psadbw have commutative source operands
On 27.05.2022 11:05, Uros Bizjak wrote:
> On Fri, May 27, 2022 at 10:13 AM Jan Beulich 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_ (commit c0569d342ca4), which
aren't commutative. Are you suggesting that the remaining (for indeed
being commutative) vgf2p8mulb_ also is incomplete,
requiring an expander as well? And maybe the same then in
v1ti3 for any_logic:V1TI, avx512bw_umulhrswv32hi3,
or _dp (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 "_psadbw"
>>[(set (match_operand:VI8_AVX2_AVX512BW 0 "register_operand" "=x,YW")
>> (unspec:VI8_AVX2_AVX512BW
>> - [(match_operand: 1 "register_operand" "0,YW")
>> + [(match_operand: 1 "register_operand" "%0,YW")
>>(match_operand: 2 "vector_operand" "xBm,YWm")]
>> UNSPEC_PSADBW))]
>>"TARGET_SSE2"
>>
>
Re: [PATCH] x86: {,v}psadbw have commutative source operands
On Fri, May 27, 2022 at 10:13 AM Jan Beulich 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 > > * config/i386/mmx.md (mmx_psadbw): Mark as commutative. > * config/i386/sse.md (_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 "_psadbw" >[(set (match_operand:VI8_AVX2_AVX512BW 0 "register_operand" "=x,YW") > (unspec:VI8_AVX2_AVX512BW > - [(match_operand: 1 "register_operand" "0,YW") > + [(match_operand: 1 "register_operand" "%0,YW") >(match_operand: 2 "vector_operand" "xBm,YWm")] > UNSPEC_PSADBW))] >"TARGET_SSE2" >
[PATCH] x86: {,v}psadbw have commutative source operands
Like noticed for gas as well (binutils-gdb commit c8cad9d389b7), the "absolute difference" aspect of the insns makes their source operands commutative. gcc/ 2022-05-XX Jan Beulich * config/i386/mmx.md (mmx_psadbw): Mark as commutative. * config/i386/sse.md (_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 "_psadbw" [(set (match_operand:VI8_AVX2_AVX512BW 0 "register_operand" "=x,YW") (unspec:VI8_AVX2_AVX512BW - [(match_operand: 1 "register_operand" "0,YW") + [(match_operand: 1 "register_operand" "%0,YW") (match_operand: 2 "vector_operand" "xBm,YWm")] UNSPEC_PSADBW))] "TARGET_SSE2"
