> -----Original Message-----
> From: Andrew Pinski <[email protected]>
> Sent: 15 June 2026 17:49
> To: [email protected]
> Cc: [email protected]; Tamar Christina <[email protected]>
> Subject: Re: [PATCH] aarch64: Fix early-ra wrong code with full-width FPR
> color groups [PR125795]
>
> On Mon, Jun 15, 2026 at 7:29 AM <[email protected]> wrote:
> >
> > From: Kyrylo Tkachov <[email protected]>
> >
> > early_ra::allocate_colors marks the FPRs occupied by a color with
> >
> > m_allocated_fprs |= ((1U << color->group->size) - 1) << best;
> >
> > When a color group spans the whole register file (size == 32), as can
> > happen for a heavily unrolled, vectorized loop, "1U << 32" is undefined
> > and evaluates to 1 sometimes, so the expression sets no bits at all.
> > The 32 FPRs of the group are therefore not recorded as allocated.
> > Subsequent colors (and broaden_colors) then reuse those registers, which
> > breaks the invariant that distinct colors receive disjoint FPRs.
> >
> > In PR125795 this let the loop-invariant TBL permute index, which is live
> > across the whole loop, share v28 with the LD2 tuple destinations, so the
> > index was clobbered mid-loop and the loop produced wrong results.
> >
> > Fix this by building the mask with a loop, which is well-defined for any
> > group size in [1, 32] (the candidate search already guarantees
> > best + size <= 32, which the patch also asserts). When the full-width
> > group can no longer be hidden, allocate_colors correctly fails to find a
> > register for the other color and the region is left to the real register
> > allocator, matching -mearly-ra=none.
> >
> > Bootstrapped and tested on aarch64-none-linux-gnu.
> >
> > I'll push it to trunk tomorrow if no objections.
> > Thanks,
> > Kyrill
> >
> > Signed-off-by: Kyrylo Tkachov <[email protected]>
> >
> > gcc/ChangeLog:
> >
> > PR target/125795
> > * config/aarch64/aarch64-early-ra.cc (early_ra::allocate_colors):
> > Build the allocated-FPR mask with a loop instead of
> > "((1U << size) - 1) << best".
> >
> > gcc/testsuite/ChangeLog:
> >
> > PR target/125795
> > * gcc.target/aarch64/pr125795.c: New test.
> > ---
> > gcc/config/aarch64/aarch64-early-ra.cc | 9 +++-
> > gcc/testsuite/gcc.target/aarch64/pr125795.c | 47
> +++++++++++++++++++++
> > 2 files changed, 55 insertions(+), 1 deletion(-)
> > create mode 100644 gcc/testsuite/gcc.target/aarch64/pr125795.c
> >
> > diff --git a/gcc/config/aarch64/aarch64-early-ra.cc
> b/gcc/config/aarch64/aarch64-early-ra.cc
> > index 24f0e1bdffb..243017e4ac1 100644
> > --- a/gcc/config/aarch64/aarch64-early-ra.cc
> > +++ b/gcc/config/aarch64/aarch64-early-ra.cc
> > @@ -3038,7 +3038,14 @@ early_ra::allocate_colors ()
> > if (dump_file && (dump_flags & TDF_DETAILS))
> > fprintf (dump_file, " Allocating [v%d:v%d] to color %d\n",
> > best, best + color->group->size - 1, color->id);
> > - m_allocated_fprs |= ((1U << color->group->size) - 1) << best;
> > + // Mark the COLOR's FPRs as allocated. Build the mask with a loop
> > + // rather than ((1U << size) - 1), since a full-width color can have
> > + // size == 32 and "1U << 32" would be undefined. The candidate
> > + // search above guarantees best + size <= 32, so each shift is in
> > + // range.
> > + gcc_assert (best + color->group->size <= 32);
> > + for (unsigned int i = 0; i < color->group->size; ++i)
> > + m_allocated_fprs |= 1U << (best + i);
>
> Why not just:
> gcc_assert (best + color->group->size <= 32);
> if (best + color->group->size == 32)
> m_allocated_fprs = -1U;
> else
> m_allocated_fprs |= ((1U << color->group->size) - 1) << best;
>
> The loop seems a bit much here.
Ikn, I prefer the loop because it's at least clear to what it's doing and
doesn't rely
on the size of m_allocated_fprs being the size of an unsigned int.
otherwise
m_allocated_fprs |= ((1ULL << color->group->size) - 1) << best;
would work too and avoid the branch entirely.
Thanks,
Tamar
>
> Thanks,
> Andrea
>
>
> > }
> > }
> >
> > diff --git a/gcc/testsuite/gcc.target/aarch64/pr125795.c
> b/gcc/testsuite/gcc.target/aarch64/pr125795.c
> > new file mode 100644
> > index 00000000000..9f5c6a32b5d
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/aarch64/pr125795.c
> > @@ -0,0 +1,47 @@
> > +/* PR target/125795 */
> > +/* { dg-do run } */
> > +/* { dg-options "-O2 -funroll-loops" } */
> > +
> > +/* This loop is vectorized with LD2 (deinterleave) and TBL (lane reversal),
> > + then unrolled. The unrolled body builds a full-width (32 FPR) early-ra
> > + color group, which used to trigger a "1U << 32" undefined shift in
> > + early_ra::allocate_colors. As a result the long-lived TBL permute index
> > + was wrongly allocated to the same FPR as an LD2 tuple destination and
> got
> > + clobbered, so the result was miscomputed. */
> > +
> > +extern void abort (void);
> > +
> > +#define N 256
> > +
> > +static int __attribute__ ((noipa))
> > +foo (void)
> > +{
> > + int k[N];
> > + int k2[N];
> > + int i;
> > +
> > + for (i = 0; i < N; i++)
> > + k2[i] = i;
> > +
> > + for (i = 0; i < 64; i++)
> > + {
> > + k[i] = k2[2 * i + 0];
> > + k[255 - i] = k2[2 * i + 1];
> > + k[64 + i] = k2[2 * i + 128];
> > + k[191 - i] = k2[2 * i + 129];
> > + }
> > +
> > + for (i = 0; i < N; i++)
> > + if (k[i] != ((i < 128) ? (2 * i) : (511 - 2 * i)))
> > + return 1;
> > +
> > + return 0;
> > +}
> > +
> > +int
> > +main (void)
> > +{
> > + if (foo ())
> > + abort ();
> > + return 0;
> > +}
> > --
> > 2.50.1 (Apple Git-155)
> >
