This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][AArch64] Fix aarch64_ira_change_pseudo_allocno_class
On Thu, May 31, 2018 at 07:23:29AM -0500, Richard Sandiford wrote:
> Wilco Dijkstra <Wilco.Dijkstra@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.
Sorry to have missed this detail in review.
> > 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?
This is OK for trunk.
Thanks,
James
> 2018-05-31 Richard Sandiford <richard.sandiford@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.