[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 Christophe Lyon changed: What|Removed |Added Assignee|clyon at gcc dot gnu.org |unassigned at gcc dot gnu.org --- Comment #25 from Christophe Lyon --- Un-assigning myself: I do not plan to work more on this for the moment.
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 Richard Biener changed: What|Removed |Added Status|ASSIGNED|NEW
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 Christophe Lyon changed: What|Removed |Added Last reconfirmed||2020-04-29 Status|UNCONFIRMED |ASSIGNED Ever confirmed|0 |1 Assignee|unassigned at gcc dot gnu.org |clyon at gcc dot gnu.org --- Comment #8 from Christophe Lyon --- Patch sent: https://gcc.gnu.org/pipermail/gcc-patches/2020-April/544872.html This is a simple improvement, hopefully simple enough for stage 4, yet useful for the end-users.
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 --- Comment #9 from Christophe Lyon --- > My initial thoughts are along the lines of... > Only try to save FP registers that this function directly clobbers. What's the point of saving these if a callee clobbers other registers? Shouldn't that be something like save-nothing vs save-all-FP-regs if there is a callee? Do you mean save direct clobbers only when the handler is a leaf function? > Provide libgcc routines to save/restore the FP context. Do you mean such routines should push all FP regs+status regs? > Or we could say simply: > interrupt routines should be compiled as if with -mgeneral-regs-only and if > they want to call some routine that uses FP then they must take it upon > themselves to save and restore the FP context.
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 --- Comment #10 from Richard Earnshaw --- (In reply to Christophe Lyon from comment #9) > > My initial thoughts are along the lines of... > > Only try to save FP registers that this function directly clobbers. > What's the point of saving these if a callee clobbers other registers? > They need to be done early enough to ensure that any code in *this* function does not clobber them. Any additional registers would have to be saved by a library call that does that. > Shouldn't that be something like save-nothing vs save-all-FP-regs if there > is a callee? > > Do you mean save direct clobbers only when the handler is a leaf function? Well, obviously if it's a leaf function, saving only the registers that are clobbered is enough, and the compiler can do the analysis to ensure that. > > > Provide libgcc routines to save/restore the FP context. > Do you mean such routines should push all FP regs+status regs? All registers that are are call clobbered. There's no need to do the call-saved registers as the compiler can do that on an as-needed case already.
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 --- Comment #11 from Christophe Lyon --- (In reply to Richard Earnshaw from comment #10) > (In reply to Christophe Lyon from comment #9) > > > My initial thoughts are along the lines of... > > > Only try to save FP registers that this function directly clobbers. > > What's the point of saving these if a callee clobbers other registers? > > > > They need to be done early enough to ensure that any code in *this* function > does not clobber them. Any additional registers would have to be saved by a > library call that does that. > Why do we need a library function for that? It would have to be special with the stack: push FP registers, but do not restore SP, so that the dual restore function can pop them and restore SP.
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 --- Comment #12 from Richard Earnshaw --- (In reply to Christophe Lyon from comment #11) > (In reply to Richard Earnshaw from comment #10) > > (In reply to Christophe Lyon from comment #9) > > > > My initial thoughts are along the lines of... > > > > Only try to save FP registers that this function directly clobbers. > > > What's the point of saving these if a callee clobbers other registers? > > > > > > > They need to be done early enough to ensure that any code in *this* function > > does not clobber them. Any additional registers would have to be saved by a > > library call that does that. > > > Why do we need a library function for that? It would have to be special with > the stack: push FP registers, but do not restore SP, so that the dual > restore function can pop them and restore SP. Because it's a lot of code to work out how many FP registers there are. You can't assume that the FPU used to compile the interrupt handler is the same as that being used at run time.
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 --- Comment #13 from Christophe Lyon --- > > Why do we need a library function for that? It would have to be special with > > the stack: push FP registers, but do not restore SP, so that the dual > > restore function can pop them and restore SP. > > Because it's a lot of code to work out how many FP registers there are. You > can't assume that the FPU used to compile the interrupt handler is the same > as that being used at run time. Ha, I had missed that point in your comment #5. I'll re-iterate on my WIP patch. But, in general (non-interrupt) code, what is supposed to happen if you compile for a d32 VFP and run on d16 one ? (and the code uses the extra registers)
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 --- Comment #14 from Richard Earnshaw --- (In reply to Christophe Lyon from comment #13) > But, in general (non-interrupt) code, what is supposed to happen if you > compile for a d32 VFP and run on d16 one ? (and the code uses the extra > registers) Well obviously that won't work. But if you build the interrupt routine with a d16 system and then call a function from it that requires d32 then that should still work if running on a d32 CPU. I think we can probably make that work, but it's probably a bit of a dance to get it all right. Hence the suggestion that this be done in a library function.
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 --- Comment #15 from Christophe Lyon --- > Well obviously that won't work. But if you build the interrupt routine with > a d16 system and then call a function from it that requires d32 then that > should still work if running on a d32 CPU. Thanks, I hadnt' thought of this combination. > > I think we can probably make that work, but it's probably a bit of a dance > to get it all right. Hence the suggestion that this be done in a library > function. I suspect I'll have to iterate a few times to get that function right: I haven't yet checked the interactions with the secure/non-secure modes, LSPEN/ASPEN (I've noticed CMSE code in GCC that takes care of FP registers). So what about adding a simple warning along the lines of comment #5 and comment #6, like the one I posted (comment #8, but maybe it should also make sure to call __aeabi_memcpy instead of memcpy?) Then a second step would allow not to use -mgeneral-regs-only and save whatever is needed. I am wondering whether we could introduce other attributes such as: - "irq-nosave-fp-regs" basically saying the user does not want to save FP registers; this would clear the warning - "irq-save-fp-regs", asking the compiler to save all the needed regs despite the penalty; this would also avoid the warning At least that would make users think about their code, but we'd needed to document that properly :-) I've noticed that several existing tests fail because of my new warning if the target defaults to float-abi=hard, depending on the default cpu/mode: gcc.misc-tests/arm-isr.c (test for excess errors) gcc.target/arm/empty_fiq_handler.c (test for excess errors) gcc.target/arm/interrupt-1.c (test for excess errors) gcc.target/arm/interrupt-2.c (test for excess errors) gcc.target/arm/pr70830.c (test for excess errors) so I'd need to change their attribute or compile them with -mgeneral-regs-only
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 --- Comment #16 from Christophe Lyon --- Another potential issue just came to my mind: what if the IRQ handler is compiled with -mfloat-abi=soft but calls a function compiled with -mfloat-abi=softfp? We have no way to guess that the FP registers can be clobbered when compiling the handler.
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 --- Comment #17 from Christophe Lyon --- (In reply to Richard Earnshaw from comment #10) > (In reply to Christophe Lyon from comment #9) > > > My initial thoughts are along the lines of... > > > Only try to save FP registers that this function directly clobbers. > > What's the point of saving these if a callee clobbers other registers? > > > > They need to be done early enough to ensure that any code in *this* function > does not clobber them. Any additional registers would have to be saved by a > library call that does that. So if this function clobbers, say d16-d17, but calls another function, do you mean we should vpush d16-d17 then call the new lib function which saves all the FP context (thus saving d16-d17 twice)? > > > Shouldn't that be something like save-nothing vs save-all-FP-regs if there > > is a callee? > > > > Do you mean save direct clobbers only when the handler is a leaf function? > > Well, obviously if it's a leaf function, saving only the registers that are > clobbered is enough, and the compiler can do the analysis to ensure that. I'm working on this, and just realized that this also means saving FPSR. It seems there's no support for that yet in arm.md (unlike aarch64.md), am I missing something? > > > > > > Provide libgcc routines to save/restore the FP context. > > Do you mean such routines should push all FP regs+status regs? > > All registers that are are call clobbered. There's no need to do the > call-saved registers as the compiler can do that on an as-needed case > already.
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 --- Comment #18 from Christophe Lyon --- > I'm working on this, and just realized that this also means saving FPSR. It > seems there's no support for that yet in arm.md (unlike aarch64.md), am I > missing something? > Sorry, I see it's called FPSCR for arm.
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 --- Comment #19 from Christophe Lyon --- (In reply to Christophe Lyon from comment #8) > Patch sent: https://gcc.gnu.org/pipermail/gcc-patches/2020-April/544872.html > > This is a simple improvement, hopefully simple enough for stage 4, yet > useful for the end-users. I have just sent an updated version of this patch: https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545747.html Maybe that would be sufficient to consider this PR fixed? Indeed I fear we open a can of worms, as I've also realized that at least some Cortex-M cores save part of the FP registers when taking an interruption, the number depends on several parameters (eg secure/non-secure, ... see "Exception entry in Cortex-M33 GUG for instance https://static.docs.arm.com/100235/0002/arm_cortex_m33_dgug_100235_0002_00_en.pdf) I haven't found such documentation for Cortex-A, so I'm not sure if they have the same behaviour. I have attached a WIP patch that demonstrates local saving of FP registers as an attachment to https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545754.html
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 --- Comment #20 from CVS Commits --- The master branch has been updated by Christophe Lyon : https://gcc.gnu.org/g:635408da1eb1d441ef4d59fe00a038c920e51085 commit r11-1062-g635408da1eb1d441ef4d59fe00a038c920e51085 Author: Christophe Lyon Date: Mon Jun 8 08:17:20 2020 + [arm] Fix vfp_operand_register for VFP HI regs 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. gcc/ChangeLog: * config/arm/predicates.md (vfp_register_operand): Use VFP_HI_REGS instead of VFP_REGS.
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 --- Comment #21 from CVS Commits --- The master branch has been updated by Christophe Lyon : https://gcc.gnu.org/g:3c3b4224875d7b8bfd4126b9dd1d9cb028997285 commit r11-1732-g3c3b4224875d7b8bfd4126b9dd1d9cb028997285 Author: Christophe Lyon Date: Tue Jun 30 13:51:20 2020 + arm: Warn if IRQ handler is not compiled with -mgeneral-regs-only [PR target/94743] The interrupt attribute does not guarantee that the FP registers are saved, which can result in problems difficult to debug. Saving the FP registers and status registers can be a large penalty, so it's probably not desirable to do that all the time. If the handler calls other functions, we'd likely need to save all of them, for lack of knowledge of which registers they actually clobber. This is even more obscure for the end-user when the compiler inserts calls to helper functions such as memcpy (some multilibs do use FP registers to speed it up). In the PR, we discussed adding routines in libgcc to save the FP context and saving only locally-clobbered FP registers, but this seems to be too much work for the purpose, given that in general such handlers try to avoid this kind of penalty. I suspect we would also want new attributes to instruct the compiler that saving the FP context is not needed. In the mean time, emit a warning to suggest re-compiling with -mgeneral-regs-only. Note that this can lead to errors if the code uses floating-point and -mfloat-abi=hard, eg: argument of type 'double' not permitted with -mgeneral-regs-only This can be troublesome for the user, but at least this would make him aware of the latent issue. The patch adds several testcases: - pr94734-1-hard.c checks that a warning is emitted when using -mfloat-abi=hard. Function IRQ_HDLR_Test can make implicit calls to runtime floating-point routines (or direct use of FP instructions), IRQ_HDLR_Test2 doesn't. We emit a warning in both cases, though. - pr94734-1-softfp.c: same as above wih -mfloat-abi=softfp. - pr94734-1-soft.c checks that no warning is emitted when using -mfloat-abi=soft when the same code as above. - pr94734-2.c checks that no warning is emitted when using -mgeneral-regs-only. - pr94734-3.c checks that no warning is emitted when using -mgeneral-regs-only even using float-point data. 2020-06-30 Christophe Lyon PR target/94743 gcc/ * config/arm/arm.c (arm_handle_isr_attribute): Warn if -mgeneral-regs-only is not used. gcc/testsuite/ * gcc.misc-tests/arm-isr.c: Add -mgeneral-regs-only. * gcc.target/arm/empty_fiq_handler.c: Add -mgeneral-regs-only. * gcc.target/arm/interrupt-1.c: Add -mgeneral-regs-only. * gcc.target/arm/interrupt-2.c: Add -mgeneral-regs-only. * gcc.target/arm/pr70830.c: Add -mgeneral-regs-only. * gcc.target/arm/pr94743-1-hard.c: New test. * gcc.target/arm/pr94743-1-soft.c: New test. * gcc.target/arm/pr94743-1-softfp.c: New test. * gcc.target/arm/pr94743-2.c: New test. * gcc.target/arm/pr94743-3.c: New test.
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 --- Comment #22 from Christophe Lyon --- Not sure if we can close this PR: I have only implemented a part of what we discussed here. GCC now emits a warning so the user can take action to make sure his code is correct/correctly generated, but GCC does not handle saving/restoring all of the FP registers automatically.
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 --- Comment #23 from CVS Commits --- The master branch has been updated by Christophe Lyon : https://gcc.gnu.org/g:2f3fd53220b74d834d300e0b7aa99eca039ffbea commit r11-1752-g2f3fd53220b74d834d300e0b7aa99eca039ffbea Author: Christophe Lyon Date: Wed Jul 1 06:48:17 2020 + arm: Fix typos in testcases [PR target/94743] In my commit r11-1732, I updated the warning message to include quotes, but I forgot to update the testcases. 2020-01-07 Christophe Lyon PR target/94743 gcc/testsuite/ * gcc.target/arm/pr94743-1-hard.c: Add missing quotes in expected warning. * gcc.target/arm/pr94743-1-softfp.c: Likewise.
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 --- Comment #24 from CVS Commits --- The master branch has been updated by Christophe Lyon : https://gcc.gnu.org/g:aa8b5ca0b540fde5890f3114f2d7076d5238fc2e commit r11-1759-gaa8b5ca0b540fde5890f3114f2d7076d5238fc2e Author: Christophe Lyon Date: Wed Jul 1 12:23:51 2020 + arm: Fix handler-align.c testcase [PR target/94743] This testcase triggers the new warning, so compile it with -mgeneral-regs-only. 2020-07-01 Christophe Lyon PR target/94743 gcc/testsuite/ * gcc.target/arm/handler-align.c: Add -mgeneral-regs-only.
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 --- Comment #1 from Christophe Lyon --- clang has implemented a warning for this case: https://reviews.llvm.org/D28820
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743
--- Comment #2 from Christophe Lyon ---
I have a preliminary patch which generates:
vpush.64{d0, d1, d2, d3, d4, d5, d6, d7}
vpush.64{d16, d17, d18, d19, d20, d21, d22, d23, d24, d25, d26,
d27, d28, d29, d30, d31}
I'm not sure users would be happy with such long push sequences
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 --- Comment #3 from Christophe Lyon --- Maybe we could - save the VFP registers as needed by default - emit a warning "IRQ handler 'foo' saves VFP registers because it is not a leaf function. If you know none of the callees will clobber the VFP registers you can use the 'IRQ-dont-push-VFP-regs' attribute" - implement this new attribute such that users can disable such long vpush sequences
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 Christophe Lyon changed: What|Removed |Added CC||ktkachov at gcc dot gnu.org --- Comment #4 from Christophe Lyon --- Adding Kyrylo so that he can share Arm's thoughts.
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 --- Comment #5 from Richard Earnshaw --- This is made more complex due to the fact that the existence of the top 16 D registers depends on the hardware you have, so saving them might require a d32 variant of the ISA, but we can't (quickly) tell in an interrupt context whether or not we have that. It only matters in reality if the interrupt routine calls a function in another translation unit where we can't see what FP registers might be needed. Also, it's not just the registers that need to be saved. The floating point status registers also need to be saved and restored. My initial thoughts are along the lines of... Only try to save FP registers that this function directly clobbers. Provide libgcc routines to save/restore the FP context. Or we could say simply: interrupt routines should be compiled as if with -mgeneral-regs-only and if they want to call some routine that uses FP then they must take it upon themselves to save and restore the FP context.
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 --- Comment #6 from Christophe Lyon --- If we consider the initial testcase, it doesn't clobber any FP register directly, but the compiler inserts a call to memcpy which does. So IIUC your 1st suggestion, it would mean: - save no FP register in the IRQ handler - call a libgcc routine to save all FP registers+status registers (this routine would have to decide about d16 vs d32 at runtime, unless we can rely on multilibs -- it would mean defining mandatory d16 and d32 libgcc multilibs...) Hence I like your simpler suggestion :-) But I think we should help the user diagnose potential problems: - maybe issue a warning when compiling an IRQ handler without -mgeneral-regs-only. That might break some packages, but would force them to check their code - also emit a warning when calling a function defined in another unit (but we should recurse) However this does not help if the compiler inserts a call to memcpy which happens to be using FPU code: the user would get a warning, but how could he solve it? Avoid implicit use of memcpy?
[Bug target/94743] IRQ handler doesn't save scratch VFP registers
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743 --- Comment #7 from Richard Earnshaw --- well, __aeabi_memcpy is required not to clobber the FP state. Sadly, GCC does not know about it...
