| Message ID | 20240116110523.2365505-1-tejas.belagod@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | AArch64: aarch64_class_max_nregs mishandles 64-bit structure modes [PR112577] | expand |
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:
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: 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));
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: