Wilco Dijkstra <wilco.dijks...@arm.com> writes:
> Richard Sandiford wrote:
>
>>> This has probably been reported elsewhere already but I can't find
>>> such a report, so sorry for possible duplicate,
>>> but this patch is causing ICEs on aarch64
>>> FAIL: gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
>>> (internal compiler error)
>>> FAIL: gcc.target/aarch64/sve/reduc_5.c -march=armv8.2-a+sve
>>> (internal compiler error)
>>>
>>> and also many scan-assembler regressions:
>>>
>>>
>>> http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/260951/report-build-info.html
>>
>> Thanks for the heads-up. Looks like they're all SVE, so I'll take this.
>
> It seems this is due to unnecessary spills of PR_REGS - the subset doesn't
> work for those.
It does, but I'd originally suggested:
if (!reg_class_subset_p (GENERAL_REGS, ...)
|| !reg_class_subset_p (FP_REGS, ...))
...bail out...
whereas the committed patch had:
if (reg_class_subset_p (..., GENERAL_REGS)
|| reg_class_subset_p (..., FP_REGS))
...bail out...
That's an important difference. The idea with the first was that
we should only make a choice between GENERAL_REGS and FP_REGS
if the original classes included both of them. And that's what
we want because the new class has to be a refinement of the
original: it shouldn't include entirely new registers.
The committed version instead says that we won't make a choice
between GENERAL_REGS and FP_REGS if one of the classes is already
specific to one of them. I think this would also lead to us changing
POINTER_REGS to GENERAL_REGS, although I don't know how much that
matters in practice.
> The original proposal doing:
>
> if (allocno_class != POINTER_AND_FP_REGS)
> return allocno_class;
>
> doesn't appear to affect SVE. However the question is whether the
> register allocator can get confused about PR_REGS and end up with
> POINTER_AND_FP_REGS for both the allocno_class and best_class? If so
> then the return needs to support predicate modes too.
Yeah, that shouldn't happen, since predicate modes are only allowed in
predicate registers.
I think the reduc_1 ICE is a separate bug that I'll post a patch for,
but it goes latent again after the patch below.
Tested on aarch64-linux-gnu. I don't think it can be called obvious
given the above, and it's only SVE-specifc by chance, so: OK to install?
Thanks,
Richard
2018-05-31 Richard Sandiford <richard.sandif...@linaro.org>
gcc/
* config/aarch64/aarch64.c (aarch64_ira_change_pseudo_allocno_class):
Fix subreg tests so that we only return a choice between
GENERAL_REGS and FP_REGS if the original classes included both.
Index: gcc/config/aarch64/aarch64.c
===================================================================
--- gcc/config/aarch64/aarch64.c 2018-05-30 19:31:14.212387813 +0100
+++ gcc/config/aarch64/aarch64.c 2018-05-31 13:12:56.836974021 +0100
@@ -1108,12 +1108,12 @@ aarch64_ira_change_pseudo_allocno_class
{
machine_mode mode;
- if (reg_class_subset_p (allocno_class, GENERAL_REGS)
- || reg_class_subset_p (allocno_class, FP_REGS))
+ if (!reg_class_subset_p (GENERAL_REGS, allocno_class)
+ || !reg_class_subset_p (FP_REGS, allocno_class))
return allocno_class;
- if (reg_class_subset_p (best_class, GENERAL_REGS)
- || reg_class_subset_p (best_class, FP_REGS))
+ if (!reg_class_subset_p (GENERAL_REGS, best_class)
+ || !reg_class_subset_p (FP_REGS, best_class))
return best_class;
mode = PSEUDO_REGNO_MODE (regno);
