| Message ID | 20231006182250.393162-1-vineetg@rivosinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [COMMITTED] RISC-V: const: hide mvconst splitter from IRA | expand |
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. 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)]
On 10/9/23 14:36, Vineet Gupta 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. > It seems Jeff recently added -fno-sched-pressure to avoid similar issues > but that apparently is no longer sufficient. I'd suggest doing a count test rather than an exact match. Verify you get a single li, two addis and one th.mveqz Jeff
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)] >
On 10/9/23 13:46, Christoph Müllner wrote: > 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. Thanks Christoph. -Vineet
On Mon, Oct 9, 2023 at 10:48 PM Vineet Gupta <vineetg@rivosinc.com> wrote: > > On 10/9/23 13:46, Christoph Müllner wrote: > > 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. > > Thanks Christoph. Tested patch on list: https://gcc.gnu.org/pipermail/gcc-patches/2023-October/632393.html > > -Vineet
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)]
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(-)