| Message ID | CAKdteObPo8CVapEZkt+cSVXFYdud_fAvxh-u8PLAKqF65pzPnw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm: Fix vfp_operand_register for VFP HI regs | expand |
Hi Christophe, > -----Original Message----- > From: Gcc-patches <gcc-patches-bounces@gcc.gnu.org> On Behalf Of > Christophe Lyon via Gcc-patches > Sent: 29 April 2020 16:53 > To: gcc Patches <gcc-patches@gcc.gnu.org> > Subject: arm: Fix vfp_operand_register for VFP HI regs > > Hi, > > While looking at PR target/94743 I noticed an ICE when I tried to save > all the FP registers: this was because all HI registers wouldn't match > vfp_register_operand. Hmm, I see that arm_regno_class indeed never returns VFP_REGS and would return VFP_HI_REGS here. So the patch looks correct to me. Do you have a testcase for the ICE to add to the testsuite? Thanks, Kyrill > > Regression-tested and bootstrapped OK. > > 2020-04-29 Christophe Lyon <christophe.lyon@linaro.org> > > gcc/ > * config/arm/predicates.md (vfp_register_operand): Use VFP_HI_REGS > instead of VFP_REGS. > > OK? > > Thanks, > > Christophe
On Wed, 29 Apr 2020 at 18:40, Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> wrote: > > Hi Christophe, > > > -----Original Message----- > > From: Gcc-patches <gcc-patches-bounces@gcc.gnu.org> On Behalf Of > > Christophe Lyon via Gcc-patches > > Sent: 29 April 2020 16:53 > > To: gcc Patches <gcc-patches@gcc.gnu.org> > > Subject: arm: Fix vfp_operand_register for VFP HI regs > > > > Hi, > > > > While looking at PR target/94743 I noticed an ICE when I tried to save > > all the FP registers: this was because all HI registers wouldn't match > > vfp_register_operand. > > Hmm, I see that arm_regno_class indeed never returns VFP_REGS and would return VFP_HI_REGS here. > So the patch looks correct to me. > Do you have a testcase for the ICE to add to the testsuite? > No C source code: I found that while extending the list of registers pushed in the prologue of an IRQ handler, more-or-less modifying arm_save_coproc_regs so that more registers are handled by vfp_emit_fstmd. The problem occurs when trying to push d16-d31. > Thanks, > Kyrill > > > > > Regression-tested and bootstrapped OK. > > > > 2020-04-29 Christophe Lyon <christophe.lyon@linaro.org> > > > > gcc/ > > * config/arm/predicates.md (vfp_register_operand): Use VFP_HI_REGS > > instead of VFP_REGS. > > > > OK? > > > > Thanks, > > > > Christophe
> -----Original Message----- > From: Christophe Lyon <christophe.lyon@linaro.org> > Sent: 30 April 2020 09:51 > To: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> > Cc: gcc-patches@gcc.gnu.org > Subject: Re: arm: Fix vfp_operand_register for VFP HI regs > > On Wed, 29 Apr 2020 at 18:40, Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> > wrote: > > > > Hi Christophe, > > > > > -----Original Message----- > > > From: Gcc-patches <gcc-patches-bounces@gcc.gnu.org> On Behalf Of > > > Christophe Lyon via Gcc-patches > > > Sent: 29 April 2020 16:53 > > > To: gcc Patches <gcc-patches@gcc.gnu.org> > > > Subject: arm: Fix vfp_operand_register for VFP HI regs > > > > > > Hi, > > > > > > While looking at PR target/94743 I noticed an ICE when I tried to save > > > all the FP registers: this was because all HI registers wouldn't match > > > vfp_register_operand. > > > > Hmm, I see that arm_regno_class indeed never returns VFP_REGS and > would return VFP_HI_REGS here. > > So the patch looks correct to me. > > Do you have a testcase for the ICE to add to the testsuite? > > > > No C source code: I found that while extending the list of registers > pushed in the prologue of an IRQ handler, more-or-less modifying > arm_save_coproc_regs so that more registers are handled by > vfp_emit_fstmd. > The problem occurs when trying to push d16-d31. I'd be comfortable taking this now for trunk (GCC 11) so it has time to bake. Once you're ready to post the IRQ handler work we can see about backporting this fix it to the branch, if we deem it necessary. Thanks, Kyrill > > > > Thanks, > > Kyrill > > > > > > > > Regression-tested and bootstrapped OK. > > > > > > 2020-04-29 Christophe Lyon <christophe.lyon@linaro.org> > > > > > > gcc/ > > > * config/arm/predicates.md (vfp_register_operand): Use > VFP_HI_REGS > > > instead of VFP_REGS. > > > > > > OK? > > > > > > Thanks, > > > > > > Christophe
On Fri, 1 May 2020 at 12:57, Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> wrote: > > > > > -----Original Message----- > > From: Christophe Lyon <christophe.lyon@linaro.org> > > Sent: 30 April 2020 09:51 > > To: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> > > Cc: gcc-patches@gcc.gnu.org > > Subject: Re: arm: Fix vfp_operand_register for VFP HI regs > > > > On Wed, 29 Apr 2020 at 18:40, Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> > > wrote: > > > > > > Hi Christophe, > > > > > > > -----Original Message----- > > > > From: Gcc-patches <gcc-patches-bounces@gcc.gnu.org> On Behalf Of > > > > Christophe Lyon via Gcc-patches > > > > Sent: 29 April 2020 16:53 > > > > To: gcc Patches <gcc-patches@gcc.gnu.org> > > > > Subject: arm: Fix vfp_operand_register for VFP HI regs > > > > > > > > Hi, > > > > > > > > While looking at PR target/94743 I noticed an ICE when I tried to save > > > > all the FP registers: this was because all HI registers wouldn't match > > > > vfp_register_operand. > > > > > > Hmm, I see that arm_regno_class indeed never returns VFP_REGS and > > would return VFP_HI_REGS here. > > > So the patch looks correct to me. > > > Do you have a testcase for the ICE to add to the testsuite? > > > > > > > No C source code: I found that while extending the list of registers > > pushed in the prologue of an IRQ handler, more-or-less modifying > > arm_save_coproc_regs so that more registers are handled by > > vfp_emit_fstmd. > > The problem occurs when trying to push d16-d31. > > I'd be comfortable taking this now for trunk (GCC 11) so it has time to bake. > Once you're ready to post the IRQ handler work we can see about backporting this fix it to the branch, if we deem it necessary. > Thanks, > Kyrill > OK, I hoped my simple-warning patch could be included in gcc-10 but I think it's now too late. (https://gcc.gnu.org/pipermail/gcc-patches/2020-April/544872.html) I'm not quite sure about the next steps, but let's continue the discussion in bugzilla. Thanks > > > > > > > Thanks, > > > Kyrill > > > > > > > > > > > Regression-tested and bootstrapped OK. > > > > > > > > 2020-04-29 Christophe Lyon <christophe.lyon@linaro.org> > > > > > > > > gcc/ > > > > * config/arm/predicates.md (vfp_register_operand): Use > > VFP_HI_REGS > > > > instead of VFP_REGS. > > > > > > > > OK? > > > > > > > > Thanks, > > > > > > > > Christophe
On Fri, 1 May 2020 at 12:57, Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> wrote: > > > > > -----Original Message----- > > From: Christophe Lyon <christophe.lyon@linaro.org> > > Sent: 30 April 2020 09:51 > > To: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> > > Cc: gcc-patches@gcc.gnu.org > > Subject: Re: arm: Fix vfp_operand_register for VFP HI regs > > > > On Wed, 29 Apr 2020 at 18:40, Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> > > wrote: > > > > > > Hi Christophe, > > > > > > > -----Original Message----- > > > > From: Gcc-patches <gcc-patches-bounces@gcc.gnu.org> On Behalf Of > > > > Christophe Lyon via Gcc-patches > > > > Sent: 29 April 2020 16:53 > > > > To: gcc Patches <gcc-patches@gcc.gnu.org> > > > > Subject: arm: Fix vfp_operand_register for VFP HI regs > > > > > > > > Hi, > > > > > > > > While looking at PR target/94743 I noticed an ICE when I tried to save > > > > all the FP registers: this was because all HI registers wouldn't match > > > > vfp_register_operand. > > > > > > Hmm, I see that arm_regno_class indeed never returns VFP_REGS and > > would return VFP_HI_REGS here. > > > So the patch looks correct to me. > > > Do you have a testcase for the ICE to add to the testsuite? > > > > > > > No C source code: I found that while extending the list of registers > > pushed in the prologue of an IRQ handler, more-or-less modifying > > arm_save_coproc_regs so that more registers are handled by > > vfp_emit_fstmd. > > The problem occurs when trying to push d16-d31. > > I'd be comfortable taking this now for trunk (GCC 11) so it has time to bake. > Once you're ready to post the IRQ handler work we can see about backporting this fix it to the branch, if we deem it necessary. > Thanks, > Kyrill > Hi, I've just sent several patches for PR 94743: https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545747.html is v2 of my previous patch: it only emits a warning, and might be sufficient to close the PR, if we decide that we don't want to save the FP registers without explicit user request... Then, [1] https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545748.html [2] https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545749.html are refactorization patches that would make implementing the patch attached here easier. If you apply the attached on top of [1] and [2], you'll notice an ICE fixed by the original patch of this thread. So hopefully applying [1], [2] and the attached should help convince you that the vfp_operand_register is OK. Thanks > > > > > > > Thanks, > > > Kyrill > > > > > > > > > > > Regression-tested and bootstrapped OK. > > > > > > > > 2020-04-29 Christophe Lyon <christophe.lyon@linaro.org> > > > > > > > > gcc/ > > > > * config/arm/predicates.md (vfp_register_operand): Use > > VFP_HI_REGS > > > > instead of VFP_REGS. > > > > > > > > OK? > > > > > > > > Thanks, > > > > > > > > Christophe
Hi, On Thu, 14 May 2020 at 17:08, Christophe Lyon <christophe.lyon@linaro.org> wrote: > > On Fri, 1 May 2020 at 12:57, Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> wrote: > > > > > > > > > -----Original Message----- > > > From: Christophe Lyon <christophe.lyon@linaro.org> > > > Sent: 30 April 2020 09:51 > > > To: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> > > > Cc: gcc-patches@gcc.gnu.org > > > Subject: Re: arm: Fix vfp_operand_register for VFP HI regs > > > > > > On Wed, 29 Apr 2020 at 18:40, Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> > > > wrote: > > > > > > > > Hi Christophe, > > > > > > > > > -----Original Message----- > > > > > From: Gcc-patches <gcc-patches-bounces@gcc.gnu.org> On Behalf Of > > > > > Christophe Lyon via Gcc-patches > > > > > Sent: 29 April 2020 16:53 > > > > > To: gcc Patches <gcc-patches@gcc.gnu.org> > > > > > Subject: arm: Fix vfp_operand_register for VFP HI regs > > > > > > > > > > Hi, > > > > > > > > > > While looking at PR target/94743 I noticed an ICE when I tried to save > > > > > all the FP registers: this was because all HI registers wouldn't match > > > > > vfp_register_operand. > > > > > > > > Hmm, I see that arm_regno_class indeed never returns VFP_REGS and > > > would return VFP_HI_REGS here. > > > > So the patch looks correct to me. > > > > Do you have a testcase for the ICE to add to the testsuite? > > > > > > > > > > No C source code: I found that while extending the list of registers > > > pushed in the prologue of an IRQ handler, more-or-less modifying > > > arm_save_coproc_regs so that more registers are handled by > > > vfp_emit_fstmd. > > > The problem occurs when trying to push d16-d31. > > > > I'd be comfortable taking this now for trunk (GCC 11) so it has time to bake. > > Once you're ready to post the IRQ handler work we can see about backporting this fix it to the branch, if we deem it necessary. > > Thanks, > > Kyrill > > > > Hi, > > I've just sent several patches for PR 94743: > https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545747.html > is v2 of my previous patch: it only emits a warning, and might be > sufficient to close the PR, if we decide that > we don't want to save the FP registers without explicit user request... > > Then, > [1] https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545748.html > [2] https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545749.html > are refactorization patches that would make implementing the patch > attached here easier. > > If you apply the attached on top of [1] and [2], you'll notice an ICE > fixed by the original patch of this thread. > > So hopefully applying [1], [2] and the attached should help convince you that > the vfp_operand_register is OK. > I've committed [1] and [2] several days ago, so it's now easier to apply the patch from https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545754.html which will trigger an ICE in the new testcases it adds, which is fixed by the patch that started this thread: https://gcc.gnu.org/pipermail/gcc-patches/2020-April/544877.html Is that lalter patch OK? Thanks, Christophe > Thanks > > > > > > > > > > > Thanks, > > > > Kyrill > > > > > > > > > > > > > > Regression-tested and bootstrapped OK. > > > > > > > > > > 2020-04-29 Christophe Lyon <christophe.lyon@linaro.org> > > > > > > > > > > gcc/ > > > > > * config/arm/predicates.md (vfp_register_operand): Use > > > VFP_HI_REGS > > > > > instead of VFP_REGS. > > > > > > > > > > OK? > > > > > > > > > > Thanks, > > > > > > > > > > Christophe
Hi Christophe, > -----Original Message----- > From: Christophe Lyon <christophe.lyon@linaro.org> > Sent: 03 June 2020 14:30 > To: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> > Cc: gcc-patches@gcc.gnu.org > Subject: Re: arm: Fix vfp_operand_register for VFP HI regs > > Hi, > > > On Thu, 14 May 2020 at 17:08, Christophe Lyon > <christophe.lyon@linaro.org> wrote: > > > > On Fri, 1 May 2020 at 12:57, Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> > wrote: > > > > > > > > > > > > > -----Original Message----- > > > > From: Christophe Lyon <christophe.lyon@linaro.org> > > > > Sent: 30 April 2020 09:51 > > > > To: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> > > > > Cc: gcc-patches@gcc.gnu.org > > > > Subject: Re: arm: Fix vfp_operand_register for VFP HI regs > > > > > > > > On Wed, 29 Apr 2020 at 18:40, Kyrylo Tkachov > <Kyrylo.Tkachov@arm.com> > > > > wrote: > > > > > > > > > > Hi Christophe, > > > > > > > > > > > -----Original Message----- > > > > > > From: Gcc-patches <gcc-patches-bounces@gcc.gnu.org> On Behalf > Of > > > > > > Christophe Lyon via Gcc-patches > > > > > > Sent: 29 April 2020 16:53 > > > > > > To: gcc Patches <gcc-patches@gcc.gnu.org> > > > > > > Subject: arm: Fix vfp_operand_register for VFP HI regs > > > > > > > > > > > > Hi, > > > > > > > > > > > > While looking at PR target/94743 I noticed an ICE when I tried to > save > > > > > > all the FP registers: this was because all HI registers wouldn't match > > > > > > vfp_register_operand. > > > > > > > > > > Hmm, I see that arm_regno_class indeed never returns VFP_REGS and > > > > would return VFP_HI_REGS here. > > > > > So the patch looks correct to me. > > > > > Do you have a testcase for the ICE to add to the testsuite? > > > > > > > > > > > > > No C source code: I found that while extending the list of registers > > > > pushed in the prologue of an IRQ handler, more-or-less modifying > > > > arm_save_coproc_regs so that more registers are handled by > > > > vfp_emit_fstmd. > > > > The problem occurs when trying to push d16-d31. > > > > > > I'd be comfortable taking this now for trunk (GCC 11) so it has time to > bake. > > > Once you're ready to post the IRQ handler work we can see about > backporting this fix it to the branch, if we deem it necessary. > > > Thanks, > > > Kyrill > > > > > > > Hi, > > > > I've just sent several patches for PR 94743: > > https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545747.html > > is v2 of my previous patch: it only emits a warning, and might be > > sufficient to close the PR, if we decide that > > we don't want to save the FP registers without explicit user request... > > > > Then, > > [1] https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545748.html > > [2] https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545749.html > > are refactorization patches that would make implementing the patch > > attached here easier. > > > > If you apply the attached on top of [1] and [2], you'll notice an ICE > > fixed by the original patch of this thread. > > > > So hopefully applying [1], [2] and the attached should help convince you > that > > the vfp_operand_register is OK. > > > > I've committed [1] and [2] several days ago, so it's now easier to apply > the patch from > https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545754.html > which will trigger an ICE in the new testcases it adds, > which is fixed by the patch that started this thread: > https://gcc.gnu.org/pipermail/gcc-patches/2020-April/544877.html This is okay. Thanks, Kyrill > > Is that lalter patch OK? > > Thanks, > > Christophe > > > Thanks > > > > > > > > > > > > > > > Thanks, > > > > > Kyrill > > > > > > > > > > > > > > > > > Regression-tested and bootstrapped OK. > > > > > > > > > > > > 2020-04-29 Christophe Lyon <christophe.lyon@linaro.org> > > > > > > > > > > > > gcc/ > > > > > > * config/arm/predicates.md (vfp_register_operand): Use > > > > VFP_HI_REGS > > > > > > instead of VFP_REGS. > > > > > > > > > > > > OK? > > > > > > > > > > > > Thanks, > > > > > > > > > > > > Christophe
diff --git a/gcc/config/arm/predicates.md b/gcc/config/arm/predicates.md index 009862e..dbd4fb4 100644 --- a/gcc/config/arm/predicates.md +++ b/gcc/config/arm/predicates.md @@ -161,7 +161,7 @@ (define_predicate "vfp_register_operand" || REGNO_REG_CLASS (REGNO (op)) == VFP_D0_D7_REGS || REGNO_REG_CLASS (REGNO (op)) == VFP_LO_REGS || (TARGET_VFPD32 - && REGNO_REG_CLASS (REGNO (op)) == VFP_REGS))); + && REGNO_REG_CLASS (REGNO (op)) == VFP_HI_REGS))); }) (define_predicate "vfp_hard_register_operand"
Hi, While looking at PR target/94743 I noticed an ICE when I tried to save all the FP registers: this was because all HI registers wouldn't match vfp_register_operand. Regression-tested and bootstrapped OK. 2020-04-29 Christophe Lyon <christophe.lyon@linaro.org> gcc/ * config/arm/predicates.md (vfp_register_operand): Use VFP_HI_REGS instead of VFP_REGS. OK? Thanks, Christophe While looking at PR target/94743 I noticed an ICE when I tried to save all the FP registers: this was because all HI registers wouldn't match vfp_register_operand. 2020-04-29 Christophe Lyon <christophe.lyon@linaro.org> gcc/ * config/arm/predicates.md (vfp_register_operand): Use VFP_HI_REGS instead of VFP_REGS.