* [COMMITTED] RISC-V: const: hide mvconst splitter from IRA
@ 2023-10-06 18:22 Vineet Gupta
[not found] ` <ea5d682f-d483-7f57-f27a-882d473026e2@rivosinc.com>
0 siblings, 1 reply; 6+ messages in thread
From: Vineet Gupta @ 2023-10-06 18:22 UTC (permalink / raw)
To: gcc-patches
Cc: kito.cheng, Jeff Law, Palmer Dabbelt, gnu-toolchain, Vineet Gupta
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)]
--
2.34.1
^ permalink raw reply [flat|nested] 6+ messages in thread[parent not found: <ea5d682f-d483-7f57-f27a-882d473026e2@rivosinc.com>]
* xthead regression with [COMMITTED] RISC-V: const: hide mvconst splitter from IRA [not found] ` <ea5d682f-d483-7f57-f27a-882d473026e2@rivosinc.com> @ 2023-10-09 20:36 ` Vineet Gupta 2023-10-09 20:45 ` Jeff Law 2023-10-09 20:46 ` Christoph Müllner 0 siblings, 2 replies; 6+ messages in thread From: Vineet Gupta @ 2023-10-09 20:36 UTC (permalink / raw) To: Christoph Müllner; +Cc: GCC Patches, Jeff Law, Patrick O'Neill 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)] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: xthead regression with [COMMITTED] RISC-V: const: hide mvconst splitter from IRA 2023-10-09 20:36 ` xthead regression with " Vineet Gupta @ 2023-10-09 20:45 ` Jeff Law 2023-10-09 20:46 ` Christoph Müllner 1 sibling, 0 replies; 6+ messages in thread From: Jeff Law @ 2023-10-09 20:45 UTC (permalink / raw) To: Vineet Gupta, Christoph Müllner; +Cc: GCC Patches, Patrick O'Neill 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: xthead regression with [COMMITTED] RISC-V: const: hide mvconst splitter from IRA 2023-10-09 20:36 ` xthead regression with " Vineet Gupta 2023-10-09 20:45 ` Jeff Law @ 2023-10-09 20:46 ` Christoph Müllner 2023-10-09 20:48 ` Vineet Gupta 1 sibling, 1 reply; 6+ messages in thread From: Christoph Müllner @ 2023-10-09 20:46 UTC (permalink / raw) To: Vineet Gupta; +Cc: GCC Patches, Jeff Law, Patrick O'Neill 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)] > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: xthead regression with [COMMITTED] RISC-V: const: hide mvconst splitter from IRA 2023-10-09 20:46 ` Christoph Müllner @ 2023-10-09 20:48 ` Vineet Gupta 2023-10-09 22:48 ` Christoph Müllner 0 siblings, 1 reply; 6+ messages in thread From: Vineet Gupta @ 2023-10-09 20:48 UTC (permalink / raw) To: Christoph Müllner; +Cc: GCC Patches, Jeff Law, Patrick O'Neill 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: xthead regression with [COMMITTED] RISC-V: const: hide mvconst splitter from IRA 2023-10-09 20:48 ` Vineet Gupta @ 2023-10-09 22:48 ` Christoph Müllner 0 siblings, 0 replies; 6+ messages in thread From: Christoph Müllner @ 2023-10-09 22:48 UTC (permalink / raw) To: Vineet Gupta; +Cc: GCC Patches, Jeff Law, Patrick O'Neill 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-10-09 22:48 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-06 18:22 [COMMITTED] RISC-V: const: hide mvconst splitter from IRA Vineet Gupta
[not found] ` <ea5d682f-d483-7f57-f27a-882d473026e2@rivosinc.com>
2023-10-09 20:36 ` xthead regression with " Vineet Gupta
2023-10-09 20:45 ` Jeff Law
2023-10-09 20:46 ` Christoph Müllner
2023-10-09 20:48 ` Vineet Gupta
2023-10-09 22:48 ` Christoph Müllner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).