[Bug 1001607] Cortex-M4F architectural Floating Point Support

[bugzilla-daemon at bugs dot ecos dot sourceware dot 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 #17 from Jonathan Larmour <jifl@ecoscentric.com> 2012-08-07 06:57:07 BST ---
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.

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.

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.).

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!.

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

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)

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

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.

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.

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.

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.

20) vectors.S still has a "NOTE:p" accidental change on line 158.

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?


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.

ÐÐÐÐÐÐÐÑÐÐ !

Jifl

-- 
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.