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

[Tejas Belagod]

On 1/24/24 5:09 PM, Richard Sandiford wrote:

Tejas Belagod <tejas.bela...@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:

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