Re: xthead regression with [COMMITTED] RISC-V: const: hide mvconst splitter from IRA

["Christoph Müllner" <christoph.muellner@vrull.eu>]

On Mon, Oct 9, 2023 at 10:36 PM Vineet Gupta <vineetg@rivosinc.com> wrote:
>
> Hi Christoph,
>
> On 10/9/23 12:06, Patrick O'Neill wrote:
> >
> > Hi Vineet,
> >
> > We're seeing a regression on all riscv targets after this patch:|
> >
> > FAIL: gcc.target/riscv/xtheadcondmov-indirect.c -O2
> > check-function-bodies ConNmv_imm_imm_reg||
> > FAIL: gcc.target/riscv/xtheadcondmov-indirect.c -O3 -g
> > check-function-bodies ConNmv_imm_imm_reg
> >
> > Debug log output:
> > body: \taddi    a[0-9]+,a[0-9]+,-1000+
> > \tli    a[0-9]+,9998336+
> > \taddi    a[0-9]+,a[0-9]+,1664+
> > \tth.mveqz    a[0-9]+,a[0-9]+,a[0-9]+
> > \tret
> >
> > against:     li    a5,9998336
> >     addi    a4,a0,-1000
> >     addi    a0,a5,1664
> >     th.mveqz    a0,a1,a4
> >     ret|
> >
> > https://github.com/patrick-rivos/gcc-postcommit-ci/issues/8
> > https://github.com/ewlu/riscv-gnu-toolchain/issues/286
> >
>
> It seems with my patch, exactly same instructions get out of order (for
> -O2/-O3) tripping up the test results and differ from say O1 for exact
> same build.
>
> -O2 w/ patch
> ConNmv_imm_imm_reg:
>      li    a5,9998336
>      addi    a4,a0,-1000
>      addi    a0,a5,1664
>      th.mveqz    a0,a1,a4
>      ret
>
> -O1 w/ patch
> ConNmv_imm_imm_reg:
>      addi    a4,a0,-1000
>      li    a5,9998336
>      addi    a0,a5,1664
>      th.mveqz    a0,a1,a4
>      ret
>
> I'm not sure if there is an easy way to handle that.
> Is there a real reason for testing the full sequences verbatim, or is
> testing number of occurrences of th.mv{eqz,nez} enough.

I did not write the test cases, I just merged two non-functional test files
into one that works without changing the actual test approach.

Given that this causes repeated issues, I think that a fall-back to counting
occurrences is the right thing to do.

I can do that if that's ok.

BR
Christoph



> It seems Jeff recently added -fno-sched-pressure to avoid similar issues
> but that apparently is no longer sufficient.
>
> Thx,
> -Vineet
>
> > Thanks,
> > Patrick
> >
> > On 10/6/23 11:22, Vineet Gupta wrote:
> >> Vlad recently introduced a new gate @ira_in_progress, similar to
> >> counterparts @{reload,lra}_in_progress.
> >>
> >> Use this to hide the constant synthesis splitter from being recog* ()
> >> by IRA register equivalence logic which is eager to undo the splits,
> >> generating worse code for constants (and sometimes no code at all).
> >>
> >> See PR/109279 (large constant), PR/110748 (const -0.0) ...
> >>
> >> Granted the IRA logic is subsided with -fsched-pressure which is now
> >> enabled for RISC-V backend, the gate makes this future-proof in
> >> addition to helping with -O1 etc.
> >>
> >> This fixes 1 addition test
> >>
> >>                 ========= Summary of gcc testsuite =========
> >>                              | # of unexpected case / # of unique unexpected case
> >>                              |          gcc |          g++ | gfortran |
> >>
> >>     rv32imac/  ilp32/ medlow |  416 /   103 |   13 /     6 |   67 /    12 |
> >>   rv32imafdc/ ilp32d/ medlow |  416 /   103 |   13 /     6 |   24 /     4 |
> >>     rv64imac/   lp64/ medlow |  417 /   104 |    9 /     3 |   67 /    12 |
> >>   rv64imafdc/  lp64d/ medlow |  416 /   103 |    5 /     2 |    6 /     1 |
> >>
> >> Also similar to v1, this doesn't move RISC-V SPEC scores at all.
> >>
> >> gcc/ChangeLog:
> >>      * config/riscv/riscv.md (mvconst_internal): Add !ira_in_progress.
> >>
> >> Suggested-by: Jeff Law<jeffreyalaw@gmail.com>
> >> Signed-off-by: Vineet Gupta<vineetg@rivosinc.com>
> >> ---
> >>   gcc/config/riscv/riscv.md | 9 ++++++---
> >>   1 file changed, 6 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
> >> index 1ebe8f92284d..da84b9357bd3 100644
> >> --- a/gcc/config/riscv/riscv.md
> >> +++ b/gcc/config/riscv/riscv.md
> >> @@ -1997,13 +1997,16 @@
> >>
> >>   ;; Pretend to have the ability to load complex const_int in order to get
> >>   ;; better code generation around them.
> >> -;;
> >>   ;; But avoid constants that are special cased elsewhere.
> >> +;;
> >> +;; Hide it from IRA register equiv recog* () to elide potential undoing of split
> >> +;;
> >>   (define_insn_and_split "*mvconst_internal"
> >>     [(set (match_operand:GPR 0 "register_operand" "=r")
> >>           (match_operand:GPR 1 "splittable_const_int_operand" "i"))]
> >> -  "!(p2m1_shift_operand (operands[1], <MODE>mode)
> >> -     || high_mask_shift_operand (operands[1], <MODE>mode))"
> >> +  "!ira_in_progress
> >> +   && !(p2m1_shift_operand (operands[1], <MODE>mode)
> >> +        || high_mask_shift_operand (operands[1], <MODE>mode))"
> >>     "#"
> >>     "&& 1"
> >>     [(const_int 0)]
>