[PATCH] AArch64: aarch64_class_max_nregs mishandles 64-bit structure modes [PR112577]

[PATCH] AArch64: aarch64_class_max_nregs mishandles 64-bit structure modes [PR112577]

[Tejas Belagod]

The target hook aarch64_class_max_nregs returns the incorrect result for 64-bit
structure modes like V31DImode or V41DFmode etc.  The calculation of the nregs
is based on the size of AdvSIMD vector register for 64-bit modes which ought to
be UNITS_PER_VREG / 2.  This patch fixes the register size.

Existing tests like gcc.target/aarch64/advsimd-intrinsics/vld1x3.c cover this change.

Regression tested on aarch64-linux. Bootstrapped on aarch64-linux.

OK for trunk?

gcc/ChangeLog:

	PR target/112577
	* config/aarch64/aarch64.cc (aarch64_class_max_nregs): Handle 64-bit
	vector structure modes correctly.
---
 gcc/config/aarch64/aarch64.cc | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index a5a6b52730d..b9f00bdce3b 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -12914,10 +12914,12 @@ aarch64_class_max_nregs (reg_class_t regclass, machine_mode mode)
 	  && constant_multiple_p (GET_MODE_SIZE (mode),
 				  aarch64_vl_bytes (mode, vec_flags), &nregs))
 	return nregs;
-      return (vec_flags & VEC_ADVSIMD
-	      ? CEIL (lowest_size, UNITS_PER_VREG)
-	      : CEIL (lowest_size, UNITS_PER_WORD));
-
+      if (vec_flags == (VEC_ADVSIMD | VEC_STRUCT | VEC_PARTIAL))
+	return GET_MODE_SIZE (mode).to_constant () / 8;
+      else
+	return (vec_flags & VEC_ADVSIMD
+		? CEIL (lowest_size, UNITS_PER_VREG)
+		: CEIL (lowest_size, UNITS_PER_WORD));
     case PR_REGS:
     case PR_LO_REGS:
     case PR_HI_REGS:
-- 
2.25.1

Re: [PATCH] AArch64: aarch64_class_max_nregs mishandles 64-bit structure modes [PR112577]

[Richard Sandiford]

Tejas Belagod <tejas.belagod@arm.com> writes:
> The target hook aarch64_class_max_nregs returns the incorrect result for 64-bit
> structure modes like V31DImode or V41DFmode etc.  The calculation of the nregs
> is based on the size of AdvSIMD vector register for 64-bit modes which ought to
> be UNITS_PER_VREG / 2.  This patch fixes the register size.
>
> Existing tests like gcc.target/aarch64/advsimd-intrinsics/vld1x3.c cover this change.
>
> Regression tested on aarch64-linux. Bootstrapped on aarch64-linux.
>
> OK for trunk?
>
> gcc/ChangeLog:
>
> 	PR target/112577
> 	* config/aarch64/aarch64.cc (aarch64_class_max_nregs): Handle 64-bit
> 	vector structure modes correctly.
> ---
>  gcc/config/aarch64/aarch64.cc | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index a5a6b52730d..b9f00bdce3b 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -12914,10 +12914,12 @@ aarch64_class_max_nregs (reg_class_t regclass, machine_mode mode)
>  	  && constant_multiple_p (GET_MODE_SIZE (mode),
>  				  aarch64_vl_bytes (mode, vec_flags), &nregs))
>  	return nregs;
> -      return (vec_flags & VEC_ADVSIMD
> -	      ? CEIL (lowest_size, UNITS_PER_VREG)
> -	      : CEIL (lowest_size, UNITS_PER_WORD));
> -
> +      if (vec_flags == (VEC_ADVSIMD | VEC_STRUCT | VEC_PARTIAL))
> +	return GET_MODE_SIZE (mode).to_constant () / 8;
> +      else
> +	return (vec_flags & VEC_ADVSIMD
> +		? CEIL (lowest_size, UNITS_PER_VREG)
> +		: CEIL (lowest_size, UNITS_PER_WORD));

Very minor, sorry, but I think it would be more usual style to add the
new condition as an early-out and so not add an "else", especially since
there's alreaedy an early-out for SVE above:

      if (vec_flags == (VEC_ADVSIMD | VEC_STRUCT | VEC_PARTIAL))
	return GET_MODE_SIZE (mode).to_constant () / 8;
      return (vec_flags & VEC_ADVSIMD
	      ? CEIL (lowest_size, UNITS_PER_VREG)
	      : CEIL (lowest_size, UNITS_PER_WORD));

I think it's also worth keeping the blank line between this and the
following block of cases.

OK with that change, thanks.

Richard


>      case PR_REGS:
>      case PR_LO_REGS:
>      case PR_HI_REGS:

Re: [PATCH] AArch64: aarch64_class_max_nregs mishandles 64-bit structure modes [PR112577]

[Tejas Belagod]

[-- Attachment #1: Type: text/plain, Size: 2452 bytes --]

On 1/24/24 5:09 PM, Richard Sandiford wrote:
> Tejas Belagod <tejas.belagod@arm.com> writes:
>> The target hook aarch64_class_max_nregs returns the incorrect result for 64-bit
>> structure modes like V31DImode or V41DFmode etc.  The calculation of the nregs
>> is based on the size of AdvSIMD vector register for 64-bit modes which ought to
>> be UNITS_PER_VREG / 2.  This patch fixes the register size.
>>
>> Existing tests like gcc.target/aarch64/advsimd-intrinsics/vld1x3.c cover this change.
>>
>> Regression tested on aarch64-linux. Bootstrapped on aarch64-linux.
>>
>> OK for trunk?
>>
>> gcc/ChangeLog:
>>
>> 	PR target/112577
>> 	* config/aarch64/aarch64.cc (aarch64_class_max_nregs): Handle 64-bit
>> 	vector structure modes correctly.
>> ---
>>   gcc/config/aarch64/aarch64.cc | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
>> index a5a6b52730d..b9f00bdce3b 100644
>> --- a/gcc/config/aarch64/aarch64.cc
>> +++ b/gcc/config/aarch64/aarch64.cc
>> @@ -12914,10 +12914,12 @@ aarch64_class_max_nregs (reg_class_t regclass, machine_mode mode)
>>   	  && constant_multiple_p (GET_MODE_SIZE (mode),
>>   				  aarch64_vl_bytes (mode, vec_flags), &nregs))
>>   	return nregs;
>> -      return (vec_flags & VEC_ADVSIMD
>> -	      ? CEIL (lowest_size, UNITS_PER_VREG)
>> -	      : CEIL (lowest_size, UNITS_PER_WORD));
>> -
>> +      if (vec_flags == (VEC_ADVSIMD | VEC_STRUCT | VEC_PARTIAL))
>> +	return GET_MODE_SIZE (mode).to_constant () / 8;
>> +      else
>> +	return (vec_flags & VEC_ADVSIMD
>> +		? CEIL (lowest_size, UNITS_PER_VREG)
>> +		: CEIL (lowest_size, UNITS_PER_WORD));
> 
> Very minor, sorry, but I think it would be more usual style to add the
> new condition as an early-out and so not add an "else", especially since
> there's alreaedy an early-out for SVE above:
> 
>        if (vec_flags == (VEC_ADVSIMD | VEC_STRUCT | VEC_PARTIAL))
> 	return GET_MODE_SIZE (mode).to_constant () / 8;
>        return (vec_flags & VEC_ADVSIMD
> 	      ? CEIL (lowest_size, UNITS_PER_VREG)
> 	      : CEIL (lowest_size, UNITS_PER_WORD));
> 
> I think it's also worth keeping the blank line between this and the
> following block of cases.
> 
> OK with that change, thanks.
> 
> Richard

Thanks for the review, Richard. Re-spin attached. Will apply.

Thanks,
Tejas.

> 
> 
>>       case PR_REGS:
>>       case PR_LO_REGS:
>>       case PR_HI_REGS:

[-- Attachment #2: dreg.txt --]
[-- Type: text/plain, Size: 698 bytes --]

diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index a5a6b52730d6c5013346d128e89915883f1707ae..a7c624f8b7327ae8c1324959c3ab5dfb4e7ebc6c 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -12914,6 +12914,8 @@ aarch64_class_max_nregs (reg_class_t regclass, machine_mode mode)
 	  && constant_multiple_p (GET_MODE_SIZE (mode),
 				  aarch64_vl_bytes (mode, vec_flags), &nregs))
 	return nregs;
+      if (vec_flags == (VEC_ADVSIMD | VEC_STRUCT | VEC_PARTIAL))
+	return GET_MODE_SIZE (mode).to_constant () / 8;
       return (vec_flags & VEC_ADVSIMD
 	      ? CEIL (lowest_size, UNITS_PER_VREG)
 	      : CEIL (lowest_size, UNITS_PER_WORD));