diff mbox series

RISC-V: unrecognizable insn ICE in xtheadvector/pr114194.c on 32bit targets

Message ID 20250209060400.2154-1-jinma@linux.alibaba.com
State Superseded
Delegated to: Kito Cheng
Headers
Series RISC-V: unrecognizable insn ICE in xtheadvector/pr114194.c on 32bit targets |

Checks

Context Check Description
rivoscibot/toolchain-ci-rivos-lint warning Lint failed
rivoscibot/toolchain-ci-rivos-apply-patch success Patch applied
rivoscibot/toolchain-ci-rivos-build--newlib-rv64gcv-lp64d-multilib success Build passed
rivoscibot/toolchain-ci-rivos-build--linux-rv64gc_zba_zbb_zbc_zbs-lp64d-multilib success Build passed
rivoscibot/toolchain-ci-rivos-build--linux-rv64gcv-lp64d-multilib success Build passed
rivoscibot/toolchain-ci-rivos-test success Testing passed
linaro-tcwg-bot/tcwg_simplebootstrap_build--master-arm-bootstrap success Build passed
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_simplebootstrap_build--master-aarch64-bootstrap success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Test passed

Commit Message

Jin Ma Feb. 9, 2025, 6:04 a.m. UTC 
  PR target/118601

gcc/ChangeLog:

	* config/riscv/riscv.cc (riscv_use_by_pieces_infrastructure_p):
	Exclude XTheadVector.

Reported-by: Edwin Lu <ewlu@rivosinc.com>
---
 gcc/config/riscv/riscv.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jin Ma Feb. 10, 2025, 8:37 a.m. UTC | #1 
On Sun, 09 Feb 2025 14:04:00 +0800, Jin Ma wrote:
> 	PR target/118601

Ok for trunk?

Best regards,
Jin Ma

> gcc/ChangeLog:
> 
> 	* config/riscv/riscv.cc (riscv_use_by_pieces_infrastructure_p):
> 	Exclude XTheadVector.
> 
> Reported-by: Edwin Lu <ewlu@rivosinc.com>
> ---
>  gcc/config/riscv/riscv.cc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 819e1538741..e5776aa0fbe 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -13826,7 +13826,7 @@ riscv_use_by_pieces_infrastructure_p (unsigned HOST_WIDE_INT size,
>    /* For set/clear with size > UNITS_PER_WORD, by pieces uses vector broadcasts
>       with UNITS_PER_WORD size pieces.  Use setmem<mode> instead which can use
>       bigger chunks.  */
> -  if (TARGET_VECTOR && stringop_strategy & STRATEGY_VECTOR
> +  if (TARGET_VECTOR && !TARGET_XTHEADVECTOR && stringop_strategy & STRATEGY_VECTOR
>        && (op == CLEAR_BY_PIECES || op == SET_BY_PIECES)
>        && speed_p && size > UNITS_PER_WORD)
>      return false;
> -- 
> 2.25.1
Kito Cheng Feb. 10, 2025, 9:09 a.m. UTC | #2 
Could you add a few more comments to explain why xtheadvector is not supported?

Maybe something like:

/* Xtheadvector doesn't support it because of the lack of XXX instrucion.  */


On Mon, Feb 10, 2025 at 4:38 PM Jin Ma <jinma@linux.alibaba.com> wrote:
>
> On Sun, 09 Feb 2025 14:04:00 +0800, Jin Ma wrote:
> >       PR target/118601
>
> Ok for trunk?
>
> Best regards,
> Jin Ma
>
> > gcc/ChangeLog:
> >
> >       * config/riscv/riscv.cc (riscv_use_by_pieces_infrastructure_p):
> >       Exclude XTheadVector.
> >
> > Reported-by: Edwin Lu <ewlu@rivosinc.com>
> > ---
> >  gcc/config/riscv/riscv.cc | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> > index 819e1538741..e5776aa0fbe 100644
> > --- a/gcc/config/riscv/riscv.cc
> > +++ b/gcc/config/riscv/riscv.cc
> > @@ -13826,7 +13826,7 @@ riscv_use_by_pieces_infrastructure_p (unsigned HOST_WIDE_INT size,
> >    /* For set/clear with size > UNITS_PER_WORD, by pieces uses vector broadcasts
> >       with UNITS_PER_WORD size pieces.  Use setmem<mode> instead which can use
> >       bigger chunks.  */
> > -  if (TARGET_VECTOR && stringop_strategy & STRATEGY_VECTOR
> > +  if (TARGET_VECTOR && !TARGET_XTHEADVECTOR && stringop_strategy & STRATEGY_VECTOR
> >        && (op == CLEAR_BY_PIECES || op == SET_BY_PIECES)
> >        && speed_p && size > UNITS_PER_WORD)
> >      return false;
> > --
> > 2.25.1
>
Craig Blackmore Feb. 11, 2025, 12:29 p.m. UTC | #3 
On 10/02/2025 08:37, Jin Ma wrote:
> On Sun, 09 Feb 2025 14:04:00 +0800, Jin Ma wrote:
>> 	PR target/118601
> 
> Ok for trunk?
> 
> Best regards,
> Jin Ma
> 
>> gcc/ChangeLog:
>>
>> 	* config/riscv/riscv.cc (riscv_use_by_pieces_infrastructure_p):
>> 	Exclude XTheadVector.
>>
>> Reported-by: Edwin Lu <ewlu@rivosinc.com>
>> ---
>>   gcc/config/riscv/riscv.cc | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
>> index 819e1538741..e5776aa0fbe 100644
>> --- a/gcc/config/riscv/riscv.cc
>> +++ b/gcc/config/riscv/riscv.cc
>> @@ -13826,7 +13826,7 @@ riscv_use_by_pieces_infrastructure_p (unsigned HOST_WIDE_INT size,
>>     /* For set/clear with size > UNITS_PER_WORD, by pieces uses vector broadcasts
>>        with UNITS_PER_WORD size pieces.  Use setmem<mode> instead which can use
>>        bigger chunks.  */
>> -  if (TARGET_VECTOR && stringop_strategy & STRATEGY_VECTOR
>> +  if (TARGET_VECTOR && !TARGET_XTHEADVECTOR && stringop_strategy & STRATEGY_VECTOR
>>         && (op == CLEAR_BY_PIECES || op == SET_BY_PIECES)
>>         && speed_p && size > UNITS_PER_WORD)
>>       return false;

`riscv_vector::expand_vec_setmem` generates the unrecognizable
instruction and your patch avoids calling it in some, but not all,
cases. Here is a case that still ICEs with `-march=rv32gc_xtheadvector
-mabi=ilp32d` and `-march=rv64gc_xtheadvector -mabi=lp64d` after
applying your patch:
```
void foo1_16 (void *p)
{
   __builtin_memset (p, 1, 16);
}
```
I suggest returning `false` in `riscv_vector::expand_vec_setmem` for
`TARGET_XTHEADVECTOR` or trying to generate something that is valid for
`TARGET_XTHEADVECTOR`. If you do bail out of
`riscv_vector::expand_vec_setmem` then you probably want to keep your
existing change too so that by pieces is still used for smaller lengths
rather than a libcall.

>> -- 
>> 2.25.1
>
Jin Ma Feb. 11, 2025, 1:34 p.m. UTC | #4 
On Tue, 11 Feb 2025 20:29:03 +0800, Craig Blackmore wrote:
> On 10/02/2025 08:37, Jin Ma wrote:
> > On Sun, 09 Feb 2025 14:04:00 +0800, Jin Ma wrote:
> >> 	PR target/118601
> > 
> > Ok for trunk?
> > 
> > Best regards,
> > Jin Ma
> > 
> >> gcc/ChangeLog:
> >>
> >> 	* config/riscv/riscv.cc (riscv_use_by_pieces_infrastructure_p):
> >> 	Exclude XTheadVector.
> >>
> >> Reported-by: Edwin Lu <ewlu@rivosinc.com>
> >> ---
> >>   gcc/config/riscv/riscv.cc | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> >> index 819e1538741..e5776aa0fbe 100644
> >> --- a/gcc/config/riscv/riscv.cc
> >> +++ b/gcc/config/riscv/riscv.cc
> >> @@ -13826,7 +13826,7 @@ riscv_use_by_pieces_infrastructure_p (unsigned HOST_WIDE_INT size,
> >>     /* For set/clear with size > UNITS_PER_WORD, by pieces uses vector broadcasts
> >>        with UNITS_PER_WORD size pieces.  Use setmem<mode> instead which can use
> >>        bigger chunks.  */
> >> -  if (TARGET_VECTOR && stringop_strategy & STRATEGY_VECTOR
> >> +  if (TARGET_VECTOR && !TARGET_XTHEADVECTOR && stringop_strategy & STRATEGY_VECTOR
> >>         && (op == CLEAR_BY_PIECES || op == SET_BY_PIECES)
> >>         && speed_p && size > UNITS_PER_WORD)
> >>       return false;
> 
> `riscv_vector::expand_vec_setmem` generates the unrecognizable
> instruction and your patch avoids calling it in some, but not all,
> cases. Here is a case that still ICEs with `-march=rv32gc_xtheadvector
> -mabi=ilp32d` and `-march=rv64gc_xtheadvector -mabi=lp64d` after
> applying your patch:
> ```
> void foo1_16 (void *p)
> {
>    __builtin_memset (p, 1, 16);
> }
> ```
> I suggest returning `false` in `riscv_vector::expand_vec_setmem` for
> `TARGET_XTHEADVECTOR` or trying to generate something that is valid for
> `TARGET_XTHEADVECTOR`. If you do bail out of
> `riscv_vector::expand_vec_setmem` then you probably want to keep your
> existing change too so that by pieces is still used for smaller lengths
> rather than a libcall.

Thank you very much for your professional reply. I think this problem is very
simple and wrong judgment has occurred. I will rethink and think about this.

Best regards,
Jin Ma

> >> -- 
> >> 2.25.1
> >
diff mbox series

Patch

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 819e1538741..e5776aa0fbe 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -13826,7 +13826,7 @@  riscv_use_by_pieces_infrastructure_p (unsigned HOST_WIDE_INT size,
   /* For set/clear with size > UNITS_PER_WORD, by pieces uses vector broadcasts
      with UNITS_PER_WORD size pieces.  Use setmem<mode> instead which can use
      bigger chunks.  */
-  if (TARGET_VECTOR && stringop_strategy & STRATEGY_VECTOR
+  if (TARGET_VECTOR && !TARGET_XTHEADVECTOR && stringop_strategy & STRATEGY_VECTOR
       && (op == CLEAR_BY_PIECES || op == SET_BY_PIECES)
       && speed_p && size > UNITS_PER_WORD)
     return false;