RISC-V: Enable TARGET_SUPPORTS_WIDE_INT

  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(-)
  

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 */