diff mbox series

x86: correct bmi2_umul<mode><dwi>3_1's MEM_P() uses

Message ID 88d618dc-0c78-81ed-5b1c-6157b9ba7a30@suse.com
State Committed
Commit c89298404071e3b42eb7e2bfbdbaa45573538b53
Headers
Series x86: correct bmi2_umul<mode><dwi>3_1's MEM_P() uses |

Commit Message

Jan Beulich May 27, 2022, 8:05 a.m. UTC 
  It's pretty clear that the operand numbers in the MEM_P() checks are
off by one, perhaps due to a copy-and-paste oversight (unlike in most
other places here we're dealing with two outputs).
---
What I don't understand is why operand 2 is "nonimmediate_operand", not
"register_operand" (which afaict would eliminate the need for these
MEM_P() checks). This would then also extend to e.g. the subsequent
umul<mode><dwi>3_1 and mul<mode><dwi>3_1 (and apparently quite a few
more).

Comments

Uros Bizjak May 27, 2022, 8:57 a.m. UTC | #1 
On Fri, May 27, 2022 at 10:05 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> It's pretty clear that the operand numbers in the MEM_P() checks are
> off by one, perhaps due to a copy-and-paste oversight (unlike in most
> other places here we're dealing with two outputs).
> ---
> What I don't understand is why operand 2 is "nonimmediate_operand", not
> "register_operand" (which afaict would eliminate the need for these
> MEM_P() checks). This would then also extend to e.g. the subsequent
> umul<mode><dwi>3_1 and mul<mode><dwi>3_1 (and apparently quite a few
> more).

Because they are commutative (due to % operand modifier) and reload
can put memory operand into each operand.

Patch is OK with the appropriate ChangeLog entry.

Thanks,
Uros.
>
> --- a/gcc/config/i386/i386.md
> +++ b/gcc/config/i386/i386.md
> @@ -8465,7 +8465,7 @@
>                         (zero_extend:<DWI> (match_dup 3)))
>             (match_operand:QI 4 "const_int_operand" "n"))))]
>    "TARGET_BMI2 && INTVAL (operands[4]) == <MODE_SIZE> * BITS_PER_UNIT
> -   && !(MEM_P (operands[1]) && MEM_P (operands[2]))"
> +   && !(MEM_P (operands[2]) && MEM_P (operands[3]))"
>    "mulx\t{%3, %0, %1|%1, %0, %3}"
>    [(set_attr "type" "imulx")
>     (set_attr "prefix" "vex")
>
Jan Beulich May 27, 2022, 10:12 a.m. UTC | #2 
On 27.05.2022 10:57, Uros Bizjak wrote:
> On Fri, May 27, 2022 at 10:05 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> It's pretty clear that the operand numbers in the MEM_P() checks are
>> off by one, perhaps due to a copy-and-paste oversight (unlike in most
>> other places here we're dealing with two outputs).
>> ---
>> What I don't understand is why operand 2 is "nonimmediate_operand", not
>> "register_operand" (which afaict would eliminate the need for these
>> MEM_P() checks). This would then also extend to e.g. the subsequent
>> umul<mode><dwi>3_1 and mul<mode><dwi>3_1 (and apparently quite a few
>> more).
> 
> Because they are commutative (due to % operand modifier) and reload
> can put memory operand into each operand.
> 
> Patch is OK with the appropriate ChangeLog entry.

Thanks, and yes, I did notice I failed to add a ChangeLog entry right
after sending (being a result of such no longer be required in
binutils, which I work more frequently with), sorry. This is what I
did add already:

gcc/
2022-05-XX  Jan Beulich  <jbeulich@suse.com>

	* config/i386/i386.md (bmi2_umul<mode><dwi>3_1): Correct MEM_P()
	arguments.

Jan
diff mbox series

Patch

--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -8465,7 +8465,7 @@ 
 			(zero_extend:<DWI> (match_dup 3)))
 	    (match_operand:QI 4 "const_int_operand" "n"))))]
   "TARGET_BMI2 && INTVAL (operands[4]) == <MODE_SIZE> * BITS_PER_UNIT
-   && !(MEM_P (operands[1]) && MEM_P (operands[2]))"
+   && !(MEM_P (operands[2]) && MEM_P (operands[3]))"
   "mulx\t{%3, %0, %1|%1, %0, %3}"
   [(set_attr "type" "imulx")
    (set_attr "prefix" "vex")