On Mon, Oct 9, 2023 at 10:36 PM Vineet Gupta <vine...@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<jeffreya...@gmail.com>
> >> Signed-off-by: Vineet Gupta<vine...@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)]
>
