| Message ID | 20220207060637.487010-1-vineetg@rivosinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V: Enable TARGET_SUPPORTS_WIDE_INT | expand |
Vineet, On Mon, 7 Feb 2022 at 07:06, Vineet Gupta <vineetg@rivosinc.com> wrote: > > This is at par with other major arches such as aarch64, i386, s390 ... > > No testsuite regressions: same numbers w/ w/o Putting that check seems a good idea, but I haven't seen any cases related to this get through anyway. Do you have seen any instances where the backend got this wrong? If so, please share, so we can run a fuller regression and see any performance impact. Thanks, Philipp. > | === gcc Summary === > | > |# of expected passes 113392 > |# of unexpected failures 27 > |# of unexpected successes 3 > |# of expected failures 605 > |# of unsupported tests 2523 > | > | === g++ Summary === > | > |# of expected passes 172997 > |# of unexpected failures 26 > |# of expected failures 706 > |# of unsupported tests 9566 > > Signed-off-by: Vineet Gupta <vineetg@rivosinc.com> > --- > gcc/config/riscv/predicates.md | 2 +- > gcc/config/riscv/riscv.c | 6 ++++++ > gcc/config/riscv/riscv.h | 2 ++ > 3 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/gcc/config/riscv/predicates.md b/gcc/config/riscv/predicates.md > index 3da6fd4c0491..cf902229954b 100644 > --- a/gcc/config/riscv/predicates.md > +++ b/gcc/config/riscv/predicates.md > @@ -52,7 +52,7 @@ > (match_test "INTVAL (op) + 1 != 0"))) > > (define_predicate "const_0_operand" > - (and (match_code "const_int,const_wide_int,const_double,const_vector") > + (and (match_code "const_int,const_wide_int,const_vector") > (match_test "op == CONST0_RTX (GET_MODE (op))"))) > > (define_predicate "reg_or_0_operand" > diff --git a/gcc/config/riscv/riscv.c b/gcc/config/riscv/riscv.c > index c830cd8f4ad1..d2f2d9e0276f 100644 > --- a/gcc/config/riscv/riscv.c > +++ b/gcc/config/riscv/riscv.c > @@ -1774,6 +1774,12 @@ riscv_rtx_costs (rtx x, machine_mode mode, int outer_code, int opno ATTRIBUTE_UN > case SYMBOL_REF: > case LABEL_REF: > case CONST_DOUBLE: > + /* With TARGET_SUPPORTS_WIDE_INT const int can't be in CONST_DOUBLE > + rtl object. Weird recheck due to switch-case fall through above. */ > + if (GET_CODE (x) == CONST_DOUBLE) > + gcc_assert (GET_MODE (x) != VOIDmode); > + /* Fall through. */ > + > case CONST: > if ((cost = riscv_const_insns (x)) > 0) > { > diff --git a/gcc/config/riscv/riscv.h b/gcc/config/riscv/riscv.h > index ff6729aedac2..91cfc82b4aa4 100644 > --- a/gcc/config/riscv/riscv.h > +++ b/gcc/config/riscv/riscv.h > @@ -997,4 +997,6 @@ extern void riscv_remove_unneeded_save_restore_calls (void); > > #define HARD_REGNO_RENAME_OK(FROM, TO) riscv_hard_regno_rename_ok (FROM, TO) > > +#define TARGET_SUPPORTS_WIDE_INT 1 > + > #endif /* ! GCC_RISCV_H */ > -- > 2.32.0 >
On 2/7/22 01:28, Philipp Tomsich wrote: > Vineet, > > On Mon, 7 Feb 2022 at 07:06, Vineet Gupta <vineetg@rivosinc.com> wrote: >> This is at par with other major arches such as aarch64, i386, s390 ... >> >> No testsuite regressions: same numbers w/ w/o > Putting that check seems a good idea, but I haven't seen any cases > related to this get through anyway. > Do you have seen any instances where the backend got this wrong? If > so, please share, so we can run a fuller regression and see any > performance impact. No, there were no failures which this fixes. Seems like other arches did this back in 2015. When aarch64 did similar change, commit 2ca5b4303bd5, a directed generic test pr68129_1.c was added which doesn't fail before/after. -Vineet
On Mon, 07 Feb 2022 09:41:10 PST (-0800), Vineet Gupta wrote: > > > On 2/7/22 01:28, Philipp Tomsich wrote: >> Vineet, >> >> On Mon, 7 Feb 2022 at 07:06, Vineet Gupta <vineetg@rivosinc.com> wrote: >>> This is at par with other major arches such as aarch64, i386, s390 ... >>> >>> No testsuite regressions: same numbers w/ w/o >> Putting that check seems a good idea, but I haven't seen any cases >> related to this get through anyway. >> Do you have seen any instances where the backend got this wrong? If >> so, please share, so we can run a fuller regression and see any >> performance impact. > > No, there were no failures which this fixes. Seems like other arches did > this back in 2015. > When aarch64 did similar change, commit 2ca5b4303bd5, a directed generic > test pr68129_1.c was added which doesn't fail before/after. The only offending MD pattern we had was for for constant 0, which IIUC should be a const_int now (and has been for some time) so shouldn't even have been matching anything. I was worried about the fcvt-based moves on rv32, but my trivial test indicates those still work fine double foo(void) { return 0; } foo: fcvt.d.w fa0,x0 ret so I'm assuming they're coming in through const_int as well. Probably worth a full rv32 testsuite run, but as far as I can tell we were essentially TARGET_SUPPORTS_WIDE_INT clean already so this should be pretty safe. Unfortunately the patch isn't trivially applying on trunk: it's targeting the wrong files and is showing some whitespace issues (though those may have been a result of me attempting to clean stuff up). I assuming that means that the tests weren't run on trunk, though. I put a cleaned up version over here <https://github.com/palmer-dabbelt/gcc/commit/1df538132d45d6d80bdb5d2dbee4d7d33606e6da.patch> in case that helps anyone. I haven't run the regressions, but otherwise Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com> LMK if you want me to run the test suite. IIUC we're still a bit away from the GCC 12 branch, and given this doesn't fix any manifestable bugs it should be held for 13. Thanks!
On 2/7/22 10:58, Palmer Dabbelt wrote: > On Mon, 07 Feb 2022 09:41:10 PST (-0800), Vineet Gupta wrote: >> >> >> On 2/7/22 01:28, Philipp Tomsich wrote: >>> Vineet, >>> >>> On Mon, 7 Feb 2022 at 07:06, Vineet Gupta <vineetg@rivosinc.com> wrote: >>>> This is at par with other major arches such as aarch64, i386, s390 ... >>>> >>>> No testsuite regressions: same numbers w/ w/o >>> Putting that check seems a good idea, but I haven't seen any cases >>> related to this get through anyway. >>> Do you have seen any instances where the backend got this wrong? If >>> so, please share, so we can run a fuller regression and see any >>> performance impact. >> >> No, there were no failures which this fixes. Seems like other arches did >> this back in 2015. >> When aarch64 did similar change, commit 2ca5b4303bd5, a directed generic >> test pr68129_1.c was added which doesn't fail before/after. > > The only offending MD pattern we had was for for constant 0, which > IIUC should be a const_int now (and has been for some time) so > shouldn't even have been matching anything. I was worried about the > fcvt-based moves on rv32, but my trivial test indicates those still > work fine > > double foo(void) > { > return 0; > } > foo: > fcvt.d.w fa0,x0 > ret > > so I'm assuming they're coming in through const_int as well. Probably > worth a full rv32 testsuite run, but as far as I can tell we were > essentially TARGET_SUPPORTS_WIDE_INT clean already so this should be > pretty safe. Ok I'll go off and run the rv32 suite just to be safe. > > Unfortunately the patch isn't trivially applying on trunk: it's > targeting the wrong files and is showing some whitespace issues > (though those may have been a result of me attempting to clean stuff > up). I assuming that means that the tests weren't run on trunk, though. I tested both gcc 11 and trunk. Both were clean. My bad that I posted the patch off of internal gcc 11 tree. > > I put a cleaned up version over here > <https://github.com/palmer-dabbelt/gcc/commit/1df538132d45d6d80bdb5d2dbee4d7d33606e6da.patch> > in case that helps anyone. I haven't run the regressions, but otherwise > > Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com> > > LMK if you want me to run the test suite. I'd be great if you can verify. I'll go off and setup a rc32 test setup as well. > IIUC we're still a bit away from the GCC 12 branch, and given this > doesn't fix any manifestable bugs it should be held for 13. Sure thing. Thx, -Vineet
On 2/7/22 13:24, Vineet Gupta wrote: > > > On 2/7/22 10:58, Palmer Dabbelt wrote: >> On Mon, 07 Feb 2022 09:41:10 PST (-0800), Vineet Gupta wrote: >>> >>> >>> On 2/7/22 01:28, Philipp Tomsich wrote: >>>> Vineet, >>>> >>>> On Mon, 7 Feb 2022 at 07:06, Vineet Gupta <vineetg@rivosinc.com> >>>> wrote: >>>>> This is at par with other major arches such as aarch64, i386, s390 >>>>> ... >>>>> >>>>> No testsuite regressions: same numbers w/ w/o >>>> Putting that check seems a good idea, but I haven't seen any cases >>>> related to this get through anyway. >>>> Do you have seen any instances where the backend got this wrong? If >>>> so, please share, so we can run a fuller regression and see any >>>> performance impact. >>> >>> No, there were no failures which this fixes. Seems like other arches >>> did >>> this back in 2015. >>> When aarch64 did similar change, commit 2ca5b4303bd5, a directed >>> generic >>> test pr68129_1.c was added which doesn't fail before/after. >> >> The only offending MD pattern we had was for for constant 0, which >> IIUC should be a const_int now (and has been for some time) so >> shouldn't even have been matching anything. I was worried about the >> fcvt-based moves on rv32, but my trivial test indicates those still >> work fine >> >> double foo(void) >> { >> return 0; >> } >> foo: >> fcvt.d.w fa0,x0 >> ret >> >> so I'm assuming they're coming in through const_int as well. Probably >> worth a full rv32 testsuite run, but as far as I can tell we were >> essentially TARGET_SUPPORTS_WIDE_INT clean already so this should be >> pretty safe. > > Ok I'll go off and run the rv32 suite just to be safe. > >> >> Unfortunately the patch isn't trivially applying on trunk: it's >> targeting the wrong files and is showing some whitespace issues >> (though those may have been a result of me attempting to clean stuff >> up). I assuming that means that the tests weren't run on trunk, though. > > I tested both gcc 11 and trunk. Both were clean. My bad that I posted > the patch off of internal gcc 11 tree. > >> >> I put a cleaned up version over here >> <https://github.com/palmer-dabbelt/gcc/commit/1df538132d45d6d80bdb5d2dbee4d7d33606e6da.patch> >> in case that helps anyone. I haven't run the regressions, but otherwise >> >> Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com> >> >> LMK if you want me to run the test suite. > > I'd be great if you can verify. I'll go off and setup a rc32 test > setup as well. Tested this for rv32 as well, no regressions w/ w/o as well - do note that actual failures between rv32 and rv64 are slightly different, but not due to this patch. > >> IIUC we're still a bit away from the GCC 12 branch, and given this >> doesn't fix any manifestable bugs it should be held for 13. > > Sure thing. > > Thx, > -Vineet
Ping ! With commit restrictions relaxed now, can this be added to trunk now ? Thx, -Vineet On 2/6/22 22:06, Vineet Gupta wrote: > This is at par with other major arches such as aarch64, i386, s390 ... > > No testsuite regressions: same numbers w/ w/o > > | === gcc Summary === > | > |# of expected passes 113392 > |# of unexpected failures 27 > |# of unexpected successes 3 > |# of expected failures 605 > |# of unsupported tests 2523 > | > | === g++ Summary === > | > |# of expected passes 172997 > |# of unexpected failures 26 > |# of expected failures 706 > |# of unsupported tests 9566 > > Signed-off-by: Vineet Gupta <vineetg@rivosinc.com> > --- > gcc/config/riscv/predicates.md | 2 +- > gcc/config/riscv/riscv.c | 6 ++++++ > gcc/config/riscv/riscv.h | 2 ++ > 3 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/gcc/config/riscv/predicates.md b/gcc/config/riscv/predicates.md > index 3da6fd4c0491..cf902229954b 100644 > --- a/gcc/config/riscv/predicates.md > +++ b/gcc/config/riscv/predicates.md > @@ -52,7 +52,7 @@ > (match_test "INTVAL (op) + 1 != 0"))) > > (define_predicate "const_0_operand" > - (and (match_code "const_int,const_wide_int,const_double,const_vector") > + (and (match_code "const_int,const_wide_int,const_vector") > (match_test "op == CONST0_RTX (GET_MODE (op))"))) > > (define_predicate "reg_or_0_operand" > diff --git a/gcc/config/riscv/riscv.c b/gcc/config/riscv/riscv.c > index c830cd8f4ad1..d2f2d9e0276f 100644 > --- a/gcc/config/riscv/riscv.c > +++ b/gcc/config/riscv/riscv.c > @@ -1774,6 +1774,12 @@ riscv_rtx_costs (rtx x, machine_mode mode, int outer_code, int opno ATTRIBUTE_UN > case SYMBOL_REF: > case LABEL_REF: > case CONST_DOUBLE: > + /* With TARGET_SUPPORTS_WIDE_INT const int can't be in CONST_DOUBLE > + rtl object. Weird recheck due to switch-case fall through above. */ > + if (GET_CODE (x) == CONST_DOUBLE) > + gcc_assert (GET_MODE (x) != VOIDmode); > + /* Fall through. */ > + > case CONST: > if ((cost = riscv_const_insns (x)) > 0) > { > diff --git a/gcc/config/riscv/riscv.h b/gcc/config/riscv/riscv.h > index ff6729aedac2..91cfc82b4aa4 100644 > --- a/gcc/config/riscv/riscv.h > +++ b/gcc/config/riscv/riscv.h > @@ -997,4 +997,6 @@ extern void riscv_remove_unneeded_save_restore_calls (void); > > #define HARD_REGNO_RENAME_OK(FROM, TO) riscv_hard_regno_rename_ok (FROM, TO) > > +#define TARGET_SUPPORTS_WIDE_INT 1 > + > #endif /* ! GCC_RISCV_H */
On Mon, 23 May 2022 14:58:29 PDT (-0700), Vineet Gupta wrote: > Ping ! With commit restrictions relaxed now, can this be added to trunk > now ? Committed, with some fixups to indentation and to handle the .c -> .cc move (which git didn't figure out for this one, not exactly sure why). > > Thx, > -Vineet > > On 2/6/22 22:06, Vineet Gupta wrote: >> This is at par with other major arches such as aarch64, i386, s390 ... >> >> No testsuite regressions: same numbers w/ w/o >> >> | === gcc Summary === >> | >> |# of expected passes 113392 >> |# of unexpected failures 27 >> |# of unexpected successes 3 >> |# of expected failures 605 >> |# of unsupported tests 2523 >> | >> | === g++ Summary === >> | >> |# of expected passes 172997 >> |# of unexpected failures 26 >> |# of expected failures 706 >> |# of unsupported tests 9566 >> >> Signed-off-by: Vineet Gupta <vineetg@rivosinc.com> >> --- >> gcc/config/riscv/predicates.md | 2 +- >> gcc/config/riscv/riscv.c | 6 ++++++ >> gcc/config/riscv/riscv.h | 2 ++ >> 3 files changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/gcc/config/riscv/predicates.md b/gcc/config/riscv/predicates.md >> index 3da6fd4c0491..cf902229954b 100644 >> --- a/gcc/config/riscv/predicates.md >> +++ b/gcc/config/riscv/predicates.md >> @@ -52,7 +52,7 @@ >> (match_test "INTVAL (op) + 1 != 0"))) >> >> (define_predicate "const_0_operand" >> - (and (match_code "const_int,const_wide_int,const_double,const_vector") >> + (and (match_code "const_int,const_wide_int,const_vector") >> (match_test "op == CONST0_RTX (GET_MODE (op))"))) >> >> (define_predicate "reg_or_0_operand" >> diff --git a/gcc/config/riscv/riscv.c b/gcc/config/riscv/riscv.c >> index c830cd8f4ad1..d2f2d9e0276f 100644 >> --- a/gcc/config/riscv/riscv.c >> +++ b/gcc/config/riscv/riscv.c >> @@ -1774,6 +1774,12 @@ riscv_rtx_costs (rtx x, machine_mode mode, int outer_code, int opno ATTRIBUTE_UN >> case SYMBOL_REF: >> case LABEL_REF: >> case CONST_DOUBLE: >> + /* With TARGET_SUPPORTS_WIDE_INT const int can't be in CONST_DOUBLE >> + rtl object. Weird recheck due to switch-case fall through above. */ >> + if (GET_CODE (x) == CONST_DOUBLE) >> + gcc_assert (GET_MODE (x) != VOIDmode); >> + /* Fall through. */ >> + >> case CONST: >> if ((cost = riscv_const_insns (x)) > 0) >> { >> diff --git a/gcc/config/riscv/riscv.h b/gcc/config/riscv/riscv.h >> index ff6729aedac2..91cfc82b4aa4 100644 >> --- a/gcc/config/riscv/riscv.h >> +++ b/gcc/config/riscv/riscv.h >> @@ -997,4 +997,6 @@ extern void riscv_remove_unneeded_save_restore_calls (void); >> >> #define HARD_REGNO_RENAME_OK(FROM, TO) riscv_hard_regno_rename_ok (FROM, TO) >> >> +#define TARGET_SUPPORTS_WIDE_INT 1 >> + >> #endif /* ! GCC_RISCV_H */
diff --git a/gcc/config/riscv/predicates.md b/gcc/config/riscv/predicates.md index 3da6fd4c0491..cf902229954b 100644 --- a/gcc/config/riscv/predicates.md +++ b/gcc/config/riscv/predicates.md @@ -52,7 +52,7 @@ (match_test "INTVAL (op) + 1 != 0"))) (define_predicate "const_0_operand" - (and (match_code "const_int,const_wide_int,const_double,const_vector") + (and (match_code "const_int,const_wide_int,const_vector") (match_test "op == CONST0_RTX (GET_MODE (op))"))) (define_predicate "reg_or_0_operand" diff --git a/gcc/config/riscv/riscv.c b/gcc/config/riscv/riscv.c index c830cd8f4ad1..d2f2d9e0276f 100644 --- a/gcc/config/riscv/riscv.c +++ b/gcc/config/riscv/riscv.c @@ -1774,6 +1774,12 @@ riscv_rtx_costs (rtx x, machine_mode mode, int outer_code, int opno ATTRIBUTE_UN case SYMBOL_REF: case LABEL_REF: case CONST_DOUBLE: + /* With TARGET_SUPPORTS_WIDE_INT const int can't be in CONST_DOUBLE + rtl object. Weird recheck due to switch-case fall through above. */ + if (GET_CODE (x) == CONST_DOUBLE) + gcc_assert (GET_MODE (x) != VOIDmode); + /* Fall through. */ + case CONST: if ((cost = riscv_const_insns (x)) > 0) { diff --git a/gcc/config/riscv/riscv.h b/gcc/config/riscv/riscv.h index ff6729aedac2..91cfc82b4aa4 100644 --- a/gcc/config/riscv/riscv.h +++ b/gcc/config/riscv/riscv.h @@ -997,4 +997,6 @@ extern void riscv_remove_unneeded_save_restore_calls (void); #define HARD_REGNO_RENAME_OK(FROM, TO) riscv_hard_regno_rename_ok (FROM, TO) +#define TARGET_SUPPORTS_WIDE_INT 1 + #endif /* ! GCC_RISCV_H */
This is at par with other major arches such as aarch64, i386, s390 ... No testsuite regressions: same numbers w/ w/o | === gcc Summary === | |# of expected passes 113392 |# of unexpected failures 27 |# of unexpected successes 3 |# of expected failures 605 |# of unsupported tests 2523 | | === g++ Summary === | |# of expected passes 172997 |# of unexpected failures 26 |# of expected failures 706 |# of unsupported tests 9566 Signed-off-by: Vineet Gupta <vineetg@rivosinc.com> --- gcc/config/riscv/predicates.md | 2 +- gcc/config/riscv/riscv.c | 6 ++++++ gcc/config/riscv/riscv.h | 2 ++ 3 files changed, 9 insertions(+), 1 deletion(-)