> On 15 Jun 2026, at 18:52, Tamar Christina <[email protected]> wrote:
>
>> -----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.
Yeah I wrote the loop just to be abundantly clear in the intent, but I think
using 1ULL is probably tighter here and should guarantee the correct behavior.
I’ll test and commit that version.
Thanks both for your inputs.
Kyrill
>
> 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)
