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

[Uros Bizjak <ubizjak@gmail.com>]

On Wed, Oct 25, 2023 at 4:41 PM Roger Sayle <roger@nextmovesoftware.com> wrote:
>
> Hi Uros,
>
> I've tried your suggestions to see what would happen.
> Alas, allowing both operands to (i386's) widening multiplications
> to be  nonimmediate_operand results in 90 additional testsuite
> unexpected failures", and 41 unresolved testcase, around things
> like:
>
> gcc.c-torture/compile/di.c:6:1: error: unrecognizable insn:
> (insn 14 13 15 2 (parallel [
>             (set (reg:DI 98 [ _3 ])
>                 (mult:DI (zero_extend:DI (mem/c:SI (plus:SI (reg/f:SI 93 virtual-stack-vars)
>                                 (const_int -8 [0xfffffffffffffff8])) [1 a+0 S4 A64]))
>                     (zero_extend:DI (mem/c:SI (plus:SI (reg/f:SI 93 virtual-stack-vars)
>                                 (const_int -16 [0xfffffffffffffff0])) [1 b+0 S4 A64]))))
>             (clobber (reg:CC 17 flags))
>         ]) "gcc.c-torture/compile/di.c":5:12 -1
>      (nil))
> during RTL pass: vregs
> gcc.c-torture/compile/di.c:6:1: internal compiler error: in extract_insn, at recog.cc:2791
>
> In my experiments, I've used nonimmediate_operand instead of general_operand,
> as a zero_extend of an immediate_operand, like const_int, would be non-canonical.
>
> In short, it's ok (common) for '%' to apply to operands with different predicates;
> reload will only swap things if the operand's predicates/constraints remain consistent.
> For example, see i386.c's *add<mode>_1 pattern.  And as shown above it can't
> be left to (until) reload to decide which "mem" gets loaded into a register (which
> would be nice), as some passes before reload check both predicates and constraints.
>
> My original patch fixes PR 110511, using the same peephole2 idiom as already
> used elsewhere in i386.md.  Ok for mainline?

Thanks for the explanation. The patch is OK.

> > -----Original Message-----
> > From: Uros Bizjak <ubizjak@gmail.com>
> > Sent: 19 October 2023 18:02
> > To: Roger Sayle <roger@nextmovesoftware.com>
> > Cc: gcc-patches@gcc.gnu.org
> > Subject: Re: [x86 PATCH] PR target/110511: Fix reg allocation for widening
> > multiplications.
> >
> > 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.

OK.

Thanks,
Uros.

> > >
> >
> >  (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.
>