[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 #41 from Jonathan Larmour <jifl@ecoscentric.com> 2012-12-17 04:02:10 GMT ---
(In reply to comment #40)
> Hi Jifl
> 
> I'm sorry for little delay, but this time you caught me in France :).

I've been overseas again recently as well, and I'm sure you don't need to worry
about delays!

[snip about option for saving fpu regs with hal_fpu_exc_push/pop 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. 
> > > That's fine. If they've enabled CYGARC_CORTEXM_FPU_EXC_AUTOSAVE we can force
> > > this new CDL option on/off (whichever way round it is implemented).
> > 
> > I don't see where this happens or did it slip through the net? I would have
> > expected the conditional which enables CYGSEM_HAL_DEBUG_FPU to also have
> > checked CYGARC_CORTEXM_FPU_EXC_AUTOSAVE.

Just to be clear, the above is a separate point to the below.

> > Incidentally, I think CYGARC_CORTEXM_FPU_EXC_AUTOSAVE should default to
> > enabled, partly because of that link you posted in comment #32:
> > http://gcc.gnu.org/ml/gcc-help/2012-10/msg00056.html which means once the FPU
> > support is enabled, GCC may start generating code which uses the FPU registers;
> 
> Fortunately this has been resolved in non issue after comment 32. And as you
> have commented just below (in reply to comment 27).

Are you referring to my comment about it not being a problem that GCC may
generate code using the FPU regs for non-FP code? Being able to disable with a
compiler flag is fine for people definitely not using FP at all. However in the
above issue, my concern would be in applications which more generally _are_
using the FPU (so have enabled CYGHWR_HAL_CORTEXM_FPU), and so they should be
able to still use the compiler optimisation in general, which means the
compiler may use FPU registers in code which has nothing to do with FP.

> > but also simply because it's more important to be safe by default. People will
> > always be able to disable it.
> 
> Provided that no floating point is used in ISR/VSR, there is no danger. I find
> it very hard [for me] to imagine application so badly needing floating point in
> ISR/DSR. On the other hand enabling the auto saving _does_ use (waste) CPU time
> and increase IRQ response time.

In the applications I refer to above, it may mean that ISR/VSR code may not use
the FPU regs, but GCC may sneakily cause them to be used after all because of
that optimisation.

But I wonder if we've been missing something here. According to
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dai0298a/DAFGGBJD.html
the Cortex-M can itself support lazy stacking, and you do enable the relevant
FPCCR bits in hal_init_fpu(). This means that the processor will lazily save
s0-s15 only if needed. We also don't need to worry about s16-s31 as the
procedure call standard means that they will automatically be saved/restored by
the compiler if it generates code which uses them. This should mean that there
is no penalty (except memory) of allowing FPU access in interrupt/exception
handlers.

I believe the only time we do need to save s16-31 is if this is a ROM monitor
or includes a GDB stub, i.e. if CYGSEM_HAL_DEBUG_FPU is defined (after the fix
we already talked about).

If you agree with my assessment (I may have missed something!), then I would
even go as far as saying that we could get rid of the _EXC_AUTOSAVE option,
because there probably isn't a reason to do anything else - i.e. we can
effectively have it permanently on.

> > > As I have a feeling that we are approaching our goal I would like to do some
> > > shaping work. One question is where is best place to put the tests. They stem
> > > from kernel tests, should I place them there or under Cortex-M architecture?
> > 
> > Definitely Cortex-M. Then the CDL which provides the list of tests
> > (CYGPKG_HAL_CORTEXM_TESTS) can also only build them if CYGPKG_KERNEL is present
> > and CYGHWR_HAL_CORTEXM_FPU on (just in case, you can see examples of how to
> > create the list of test names around the place in eCos, e.g. libc stdio).
> > 
> > The tests themselves may also need fine-tuning of more detailed configuration
> > options and can use CYG_TEST_NA for that if they are not applicable for the
> > configuration after all.
> 
> I think they should be applicable to all configurations that have enough
> memory.

Well... they all unconditionally include CYGPKG_KERNEL specific headers, so
must only be built if CYGPKG_KERNEL is enabled (which was my point).

But there are other issues with the tests, especially if placed outside of the
cortex-M HAL:-
 - fpinttest.c includes a call to CYGARC_MRS().

 - thread_switch_fpu.cxx's HAL_CLOCK_READ_NS is cortex-m specific

 - There are dependencies on strlen(), strcat(), fabs(), strcpy() at least - if
you want to use the compiler builtins, use e.g. __builtin_strlen instead).
Otherwise the functions may not be declared, or for fabs() <math.h> won't
exist.

 - alarm_fn calls time(), which obviously has no compiler builtin equivalent.

 - In thread_fp() in thread_switch_fpu.cxx: yes, GCC can optimise that out. It
is even allowed to do constant folding like that with no optimisation flag
(-O*). It didn't use to be the case for floating point constant, but now is (I
can't remember whether it's before or after 4.3, but it's true now).

 - I'm not sure I see the value in fpinttest.c in addition to fptestf.c?


> They execute without problem on systems without FPU, and at least
> thread_switch_fpu. is (IMO) gives interesting comparisons when run with and
> without FPU.

Hooray :).

> > Just a couple more little comments:
> > 
> > CYGHWR_HAL_FPV4_SP_D16 should be an option, not a component, and should
> > probably be a "calculated 1" bool rather than "flavor none".
> > 
> 
> Yes, it's nicer that way. I tried to make it look little bit more informative:
> 
>    cdl_option CYGHWR_HAL_FPV4_SP_D16 {
>        flavor bool
>        active_if { CYGINT_HAL_FPV4_SP_D16 }
>        calculated { CYGINT_HAL_FPV4_SP_D16 && CYGHWR_HAL_CORTEXM_FPU }
>        ...
>   }

Come to think of it, do we need CYGHWR_HAL_FPV4_SP_D16 at all? Can we not just
use CYGINT_HAL_FPV4_SP_D16 directly? And then make
CYGBLD_ARCH_CPUFLAG_FPV4SPD16 use (CYGHWR_HAL_CORTEXM_FPU &&
CYGINT_HAL_FPV4_SP_D16) instead? As far as I can tell that component is the
only user of that setting.

> Out of topic: One question, just out of curiosity: is there a need in CDL for
> cdl_option keyword? Isn't it just a terminal cdl_component?

There sort-of is, because it helps the UI. Options can have explicit parents,
including in other packages (the serial drivers have the most common example of
this). This means it's worth knowing whether something is really a component if
it is allowed to contain options underneath it, even if it currently doesn't.
Theoretically the UI could work out that it has to represent an option as what
we currently call a component if it has any children; however if we did that,
a) it would make some things intended as components look like options even
though they shouldn't; and b) allowing you to choose any option as a parent
would make it easier to make mistakes. So instead it's explicit which options
(i.e. components) are allowed to contain other options.

It's true there's no technical reason, only design reasons.

> What about adding 
> 
> #define CYGBLD_INLINE __attribute__((always_inline))
> 
> in infra/current/include/cyg_type.h?
> Note: Proposal of the name above instead of CYGBLD_ATTRIB_ALWAYS_INLINE is
> intentional.

I don't think that name is wise because "inline" can already mean lots of
different things. It has different meanings in old GCC, C99, C++, and if
prefaced with 'static' or 'extern'. Calling something just 'inline' isn't
enough.

What's your objection to CYGBLD_ATTRIB_ALWAYS_INLINE? Would
CYGBLD_ATTRIB_FORCE_INLINE be better? Or is it the length?

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.