> On 15 Jun 2026, at 20:35, Alexander Monakov <[email protected]> wrote: > > > On Mon, 15 Jun 2026, Tamar Christina wrote: > >>>> --- 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. > > I would agree. > >> 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. > > I'm not sure how, it still relies on 1u (and not 1ull) being sufficiently wide > in '1U << (best + i)'. > >> otherwise >> >> m_allocated_fprs |= ((1ULL << color->group->size) - 1) << best; >> >> would work too and avoid the branch entirely. > > Surely we can find a nicer way to express this, for instance: > > ~(~0u << color->group->size) << best
FWIW I think Tamar’s version is a bit more readable, though at this point this is a subjective cosmetic choice. Given I’ve tested Tamar’s version and this is a serious-ish wrong-code bug I’ll go ahead and commit that. Thanks, Kyrill > > Alexander
