Wilco Dijkstra <wilco.dijks...@arm.com> writes:
> Richard Sandiford wrote:
>> - if (allocno_class != ALL_REGS)
>> + if (allocno_class != POINTER_AND_FP_REGS)
>> return allocno_class;
>>
>> - if (best_class != ALL_REGS)
>> + if (best_class != POINTER_AND_FP_REGS)
>> return best_class;
>>
>> mode = PSEUDO_REGNO_MODE (regno);
>
>> I think it'd be better to use !reg_class_subset_p (POINTER_AND_FP_REGS, ...)
>> instead of ... != POINTER_AND_FP_REGS, since this in principle still applies
>> to ALL_REGS too.
>>
>> FWIW, the patch looks good to me with that change.
>
> How does reg_class_subset_p help? In my testing I didn't see ALL_REGS ever
> used (and I don't believe it's possible to get it with SVE either). And
> it's not obvious
> without looking at the implementation whether subset here means strict
> subset or not,
> so it would obfuscate the clear meaning of the existing patch.
But I think the fact that we need this patch shows why hard-coding the
names of union classes is dangerous. IMO the question isn't whether we
see ALL_REGS used but whether there's a reason in principle why it
wouldn't be used. E.g. ALL_REGS is the starting class for the
best_class calculation, and LRA uses ALL_REGS as the default choice
for scratch reload registers.
It's not like we can claim that the testsuite will flag up if this
goes wrong again, since AIUI there are no tests that show the reason
we need to make this change. (I realise the patch includes an md
change to keep the testsuite happy, but that's not the same thing.
I mean more a test that shows why removing the '*'s made things
worse, through no fault of its own.)
Conceptually what we're saying here is that if the given classes
include both GENERAL_REGS and FP_REGS, we'll choose between them
based on the mode of the register. And that makes sense for any
class that includes both GENERAL_REGS and FP_REGS. We could write
it that way if it seems better, i.e.:
if (!reg_class_subset_p (GENERAL_REGS, ...)
|| !reg_class_subset_p (FP_REGS, ...))
...
That way we don't mention any union classes, and I think the meaning
is clear in the context of eventually returning GENERAL_REGS or FP_REGS.
reg_class_subset_p tests for the normal inclusive subset relation
rather than "strict subset".
Thanks,
Richard
