Re: [x86 PATCH] PR target/110511: Fix reg allocation for widening multiplications.

[Uros Bizjak <ubizjak@gmail.com>]

On Tue, Oct 17, 2023 at 9:05 PM Roger Sayle <roger@nextmovesoftware.com> wrote:
>
>
> This patch contains clean-ups of the widening multiplication patterns in
> i386.md, and provides variants of the existing highpart multiplication
> peephole2 transformations (that tidy up register allocation after
> reload), and thereby fixes PR target/110511, which is a superfluous
> move instruction.
>
> For the new test case, compiled on x86_64 with -O2.
>
> Before:
> mulx64: movabsq $-7046029254386353131, %rcx
>         movq    %rcx, %rax
>         mulq    %rdi
>         xorq    %rdx, %rax
>         ret
>
> After:
> mulx64: movabsq $-7046029254386353131, %rax
>         mulq    %rdi
>         xorq    %rdx, %rax
>         ret
>
> The clean-ups are (i) that operand 1 is consistently made register_operand
> and operand 2 becomes nonimmediate_operand, so that predicates match the
> constraints, (ii) the representation of the BMI2 mulx instruction is
> updated to use the new umul_highpart RTX, and (iii) because operands
> 0 and 1 have different modes in widening multiplications, "a" is a more
> appropriate constraint than "0" (which avoids spills/reloads containing
> SUBREGs).  The new peephole2 transformations are based upon those at
> around line 9951 of i386.md, that begins with the comment
> ;; Highpart multiplication peephole2s to tweak register allocation.
> ;; mov imm,%rdx; mov %rdi,%rax; imulq %rdx  ->  mov imm,%rax; imulq %rdi
>
>
> This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
> and make -k check, both with and without --target_board=unix{-m32}
> with no new failures.  Ok for mainline?
>
>
> 2023-10-17  Roger Sayle  <roger@nextmovesoftware.com>
>
> gcc/ChangeLog
>         PR target/110511
>         * config/i386/i386.md (<u>mul<mode><dwi>3): Make operands 1 and
>         2 take "regiser_operand" and "nonimmediate_operand" respectively.
>         (<u>mulqihi3): Likewise.
>         (*bmi2_umul<mode><dwi>3_1): Operand 2 needs to be register_operand
>         matching the %d constraint.  Use umul_highpart RTX to represent
>         the highpart multiplication.
>         (*umul<mode><dwi>3_1):  Operand 2 should use regiser_operand
>         predicate, and "a" rather than "0" as operands 0 and 2 have
>         different modes.
>         (define_split): For mul to mulx conversion, use the new
>         umul_highpart RTX representation.
>         (*mul<mode><dwi>3_1):  Operand 1 should be register_operand
>         and the constraint %a as operands 0 and 1 have different modes.
>         (*<u>mulqihi3_1): Operand 1 should be register_operand matching
>         the constraint %0.
>         (define_peephole2): Providing widening multiplication variants
>         of the peephole2s that tweak highpart multiplication register
>         allocation.
>
> gcc/testsuite/ChangeLog
>         PR target/110511
>         * gcc.target/i386/pr110511.c: New test case.
>

 (define_insn "*bmi2_umul<mode><dwi>3_1"
   [(set (match_operand:DWIH 0 "register_operand" "=r")
     (mult:DWIH
-      (match_operand:DWIH 2 "nonimmediate_operand" "%d")
+      (match_operand:DWIH 2 "register_operand" "%d")
       (match_operand:DWIH 3 "nonimmediate_operand" "rm")))
    (set (match_operand:DWIH 1 "register_operand" "=r")

This will fail. Because of %, both predicates must allow
nonimmediate_operand, since RA can swap operand constraints due to %.

@@ -9747,7 +9743,7 @@
   [(set (match_operand:<DWI> 0 "register_operand" "=r,A")
     (mult:<DWI>
       (zero_extend:<DWI>
-        (match_operand:DWIH 1 "nonimmediate_operand" "%d,0"))
+        (match_operand:DWIH 1 "register_operand" "%d,a"))
       (zero_extend:<DWI>
         (match_operand:DWIH 2 "nonimmediate_operand" "rm,rm"))))
    (clobber (reg:CC FLAGS_REG))]

The same here, although changing "0" to "a" is correct, but the
oversight is benign. "A" reads as "ad", and the first alternative
already takes "d".

+;; Widening multiplication peephole2s to tweak register allocation.
+;; mov imm,%rdx; mov %rdi,%rax; mulq %rdx  ->  mov imm,%rax; mulq %rdi

Maybe instead of peephole2s, we allow general_operands in the
instruction (both inputs) and rely on the RA to fulfill the
constraint? Would this work?

Uros.