[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon@bugs.ecos.sourceware.org]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

--- Comment #19 from Ilija Kocho <ilijak@siva.com.mk> 2012-08-08 18:14:13 BST ---
(In reply to comment #17)
> More comments....
> 
> 7) There's a few more things from cortexm_stub.h which you put in fpv4_sp_d16.h
> which I don't think need to be: HAL_STUB_REGISTERS_SIZE, PS_N/Z/C/V, the
> definition of target_register_t and TARGET_HAS_LARGE_REGISTERS. These
> definitions will be the same everywhere in Cortex-M so can just live once in
> cortexm_stub.h.

OK.

> 
> 8) In fpv4_sp_d16.h, hal_cortexm_fpu_enable/disable are still extern inline,
> which as I mentioned in comment #9 can be problematic. You are slightly at risk
> of link errors depending on the compiler optimisations. So these should be
> HAL_CORTEXM_FPU_ENABLE/DISABLE preprocessor macros instead.

I haven't experienced problems so far, but I haven't tried other optimization
than default. But we can trade them for macros, though I am reluctant.

> 
> 9) CYGARC_CORTEXM_FPU_EXC_AUTOSAVE defaults to off.
> 
> 10) A bit like my previous comment about lining up things in CDL, lining up
> some things here can be clearer too. For example:
> #  define HAL_SAVEDREG_MAN_EXCEPTION_FPU_N 16
> #  define HAL_SAVEDREG_AUTO_EXCEPTION_FPU_N 16
> #  define HAL_SAVEDREG_AUTO_FRAME_SIZE (8*4 + 16*4 + 4 + 4)
> 
> // HAL_SavedRegisters entries for floating point registers
> //     see hal_arch.h for HAL_SavedRegisters definition.
> 
> #  define HAL_SAVEDREG_AUTO_FPU_EXCEPTION_S                           \
>             cyg_uint32          s_auto[HAL_SAVEDREG_AUTO_EXCEPTION_FPU_N]; \
>             cyg_uint32          fpscr;                               \
>             cyg_uint32          align;
> 
> is a little better as:
> 
> #  define HAL_SAVEDREG_MAN_EXCEPTION_FPU_N    16
> #  define HAL_SAVEDREG_AUTO_EXCEPTION_FPU_N   16
> #  define HAL_SAVEDREG_AUTO_FRAME_SIZE        (8*4 + 16*4 + 4 + 4)
> 
> // HAL_SavedRegisters entries for floating point registers
> //     see hal_arch.h for HAL_SavedRegisters definition.
> 
> #  define HAL_SAVEDREG_AUTO_FPU_EXCEPTION_S                                \
>             cyg_uint32          s_auto[HAL_SAVEDREG_AUTO_EXCEPTION_FPU_N]; \
>             cyg_uint32          fpscr;                                     \
>             cyg_uint32          align;
> 
> Well, personally I find it easier to read anyway.
> 
> 11) hal_arch.inc: The line indentation isn't very consistent through this,
> including of comments. It may also be helpful to put a comment separator
> between the two halves (i.e. //=========================== etc.).

I'll try to improve.

> 
> 12) hal_fpu_unenable surely ought to be named just hal_fpu_disable ? Ditto all
> the other mentions of unenable. Just wondering if you're using it specifically
> or just temporarily forgot the word :-). Bear in mind I don't know a single
> word of Macedonian so I'm grateful your English is so good!.
> 

My English is less than perfect but here it is not a lost memory. The meaning
is (intended to be), as stated in the comment just below the line
_undo_last_enable_. It is not equivalent to disable because the FPU might have
been enabled already.

> 13) In fpv4_sp_d16.h the uses of __regs_p in HAL_THREAD_INIT_FPU_REGS and
> _CONTEXT still need brackets for safety :).
> 

One reason why I prefer functions to macros.

> 14) In hal_arch.h, CYGNUM_HAL_STACK_CONTEXT_SIZE still needs abstracting so
> that different FPUs can have different sizes (as per comment #9)
> 

OK. Then it (or override) shall be defined in fpv4_sp_d16.h


> 15) In hal_arch.inc, hal_fpu_enable and _disable should use
> CYGARC_REG_FPU_CPACR_ENABLE to be consistent

OK. There are some other places as well.

> 
> 16) The banner at the top of hal_arch.inc isn't right (it says vectors.S) and
> the description of cortexm_fpu.c could be better - I think that was just taken
> from vectors.S as well.

Yes, it is.

> 
> 17) As mentioned in comment #9, I think all the usage fault processing in
> vectors.S and hal_misc.c should only be present if lazy FPU context switching
> is enabled. Otherwise let it default to the hal_default_exception_vsr. That's
> the only time we need it.

OK.

> 
> 18) Before, I had mentioned about saving FP context with interrupts and
> exceptions. I was making two observations there, one was about interrupts
> (thanks for sorting that out with LSPEN/ASPEN) but the other is that, unless
> the user explicitly requests it, we should only save the FP state for
> exceptions if we're a ROM monitor or have GDB stubs. At the moment they're
> always saved, which I think is unnecessary for most people's production
> applications. So I think we need an option just to cover the
> hal_fpu_exc_push/pop calls in hal_default_exception_vsr.

It is only possible when automatic FPU context saving is disabled. With enabled
automatic FPU context saving we have no choice. 

> 
> 19) reg_offset() in cortexm_stub.c still has a diag_printf. It also would be
> improved with some abstraction. I _think_ the following untested code would do
> it and then we wouldn't need two different versions at all:
> static int
> reg_offset(regnames_t reg)
> {
>     int i, offset;
>     for (i=0,offset=0; i<NUMREGS; i++)
>     {
>         if ( i == reg )
>             break;
>         offset += REGSIZE(i);
>     }
>     if ( NUMREGS == i || REGSIZE(i) == 0 )
>         return -1;
>     return offset;
> }
> since this is only used by the stub, a tiny amount of runtime inefficiency is
> fine.
> 

I need some time to check this.

> 
> 21) It looks like we still may not agree about whether usage faults should have
> their own exception vsr or not. As I said they are only used with lazy FPU
> handling and at most once per thread, so this is not performance critical code.
> Code size might be critical though. But the biggest problem is that, as you
> say, at present the hal_usagefault_exception_vsr has a different
> prologue/epilogue to hal_default_exception_vsr. That means the user can't debug
> any usage faults such as illegal instructions or unaligned accesses because the
> stub won't be entered. Whether with a separate vsr or not, I think it is
> important we have a solution that still allows users to debug usage faults. I
> guess that if we did do this through hal_default_exception_vsr then it would
> mean having to test whether the FPU was enabled or not before attempting to
> save the context. On the other hand, exceptions are of course rare and usually
> not something programs rely on for normal operation (only fatal errors
> usually), so performance probably isn't critical either. Can you think of any
> other way to allow usage faults to be debuggable when using the usage fault
> exception and lazy FPU context switching?
> 

It is possible to change hal_deliver_usagefault_exception to return a value
that will be !0 if there are still exceptions to process. Then
hal_usagefault_exception_vsr can be tweaked to jump into
hal_default_exception_vsr.  I am testing this and am going to post a patch
later, here is a snippet:

=== Code snippet =================
hal_usagefault_exception_vsr:

        mrs     r0,psp                  // Get process stack
        sub     r1,r0,#(12*4)           // Make space for saved state
        msr     psp,r1                  // Ensure PSP is up to date
        mrs     r3,basepri              // R3 = basepri
        stmfd   r0!,{r2-r11,lr}         // save registers
        sub     r0,#4
        mov     r4,r0                   // R4 = saved state pointer

        bl      hal_deliver_usagefault_exception
        mov     r1,r0                   // return code

        mov     r0,r4                   // R0 = state saved across call
        add     r0,#4
        ldmfd   r0!,{r2-r11,lr}         // Restore registers
        msr     psp,r0                  // Restore PSP
        msr     basepri,r3              // Restore basepri

        cmp     r1,#0                   // Exc. other than FPU?
        bne     hal_default_exception_vsr // Yes - process it

        bx      lr                      // Not: Return

====== Snippet end ====================

> 
> I'm prepared to make many of these changes as it is unfair of me to ask you to
> do them all when you have already done so much, but given how much time has
> passed since this patch, I'm not sure if you have made other changes in the
> meantime, so let me know if you want me to make changes, so we can avoid
> clashes and having to merge.

If you have time that would be helpful, especially with CDL, but before we jump
to coding let's discuss it. I haven't changed anything since I posted the last
patch as I expected your comments. Now I'm testing the usagefault tweak and I
plan to post a small incremental patch later today or tomorrow.

> 
> Благодарам !

So you do some Macedonian after all :-)

Cheers

Ilija

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.