[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 #40 from Ilija Kocho <ilijak@siva.com.mk> 2012-12-16 15:51:37 GMT ---
Hi Jifl

I'm sorry for little delay, but this time you caught me in France :).

(In reply to comment #39)
> Hi Ilija,
> 
> Excuse the length of this, since I have a few older things I never commented
> on. On the plus side, we're nearly there! Everything below is easy to sort out
> I think.

Thank you for extended elaboration. It seems that for many issues we have
consensus so they have been snipped out.

> > 18) I added condition to suppress FPU context saving for exceptions when not
> > selected ROM monitor or STUB.
> 
> Ah, but in cortexm_fpu.h you define CYGSEM_HAL_DEBUG_FPU if
> CYGSEM_HAL_USE_ROM_MONITOR is set. But I don't think that one should cause it
> to be set. CYGSEM_HAL_ROM_MONITOR yes, but not USE_ROM_MONITOR.
> 

OK. Now corrected.

> (In reply to comment #19)
> >>> 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. 
> > 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.
> 
> 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).

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

I would rather keep this option disabled by default, and those few that may
desperately need it, can easily enable it.

> 
> 
> (In reply to comment #27)

[snip]

> Joey Ye's reply did also say that whatever he would do would be able to be
> disabled (or maybe wouldn't be enabled by default), e.g. with
> -fno-fpreg-for-nonfpdata. So I don't think we need to worry.
> 
> (In reply to comment #33)

[snip]

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

> 
> (In reply to comment #36)
> > Changes: Default setting now is SOFT FPU. Rationale is that more often people
> > will make integer applications even on systems with FPU. I may be completely
> > wrong so comments are appreciated.
> 
> No I think you're right. There are other reasons to use the STM32F4 than the
> FPU, for example.

OK. Then we keep it as is.

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

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?


> In case you want to change your mind about HAL_CORTEXM_FPU_ENABLE/DISABLE being
> macros instead of inline functions, I have belatedly realised it's possible to
> use a GCC attribute: __attribute__((always_inline)). That would allow you to
> keep them as inline functions if you like. I'll leave it up to you if you want
> to go back and change it, given that what's there now works. I'm happy either
> way.

I would not fix something that works, but I think there are some other inline
functions that I could treat.

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.

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.