* [PATCH] AArch64: aarch64_class_max_nregs mishandles 64-bit structure modes [PR112577] @ 2024-01-16 11:05 Tejas Belagod 2024-01-24 11:39 ` Richard Sandiford 0 siblings, 1 reply; 3+ messages in thread From: Tejas Belagod @ 2024-01-16 11:05 UTC (permalink / raw) To: gcc-patches; +Cc: Tejas Belagod, richard.sandiford 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] AArch64: aarch64_class_max_nregs mishandles 64-bit structure modes [PR112577] 2024-01-16 11:05 [PATCH] AArch64: aarch64_class_max_nregs mishandles 64-bit structure modes [PR112577] Tejas Belagod @ 2024-01-24 11:39 ` Richard Sandiford 2024-02-06 5:47 ` Tejas Belagod 0 siblings, 1 reply; 3+ messages in thread From: Richard Sandiford @ 2024-01-24 11:39 UTC (permalink / raw) To: Tejas Belagod; +Cc: gcc-patches 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: ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] AArch64: aarch64_class_max_nregs mishandles 64-bit structure modes [PR112577] 2024-01-24 11:39 ` Richard Sandiford @ 2024-02-06 5:47 ` Tejas Belagod 0 siblings, 0 replies; 3+ messages in thread From: Tejas Belagod @ 2024-02-06 5:47 UTC (permalink / raw) To: gcc-patches, richard.sandiford [-- 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)); ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-02-06 5:47 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-01-16 11:05 [PATCH] AArch64: aarch64_class_max_nregs mishandles 64-bit structure modes [PR112577] Tejas Belagod 2024-01-24 11:39 ` Richard Sandiford 2024-02-06 5:47 ` Tejas Belagod
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).