[COMMITTED] RISC-V: const: hide mvconst splitter from IRA
Checks
Commit Message
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(-)
Comments
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
@@ -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)]