[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 #43 from Ilija Kocho <ilijak@siva.com.mk> 2012-12-17 15:30:36 GMT ---
(In reply to comment #41)
> (In reply to comment #40)

[snip]

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

I am referring to Joey Ye's statement that GCC does not use VFP registers for
non-FP code: http://gcc.gnu.org/ml/gcc-help/2012-10/msg00056.html
and that once added, such optimization shall be accompanied with flag that
disables it. In such future we shall add (-fno-fpreg-for-nonfpdata) to CFLAGS.

[snip, replied in comment 42]

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

I was not clear enough, sorry. I haven't stated that I have agreed for tests to
live in Cortex-M domain. My statement "applicable to all configurations ..." is
in that context.

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

I'll look at it.

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

For this test it's only important for code to touch a VFP register. It does so
for my builds (GCC 4.6.3 and 4.7.2 with default eCos flags), but yes we should
be careful.

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

A replacement rather than addition. fpinttest.c runs a mix of pure integer and
float threads that enforces LAZY switches. It detected bugs (in LAZY context
switching scheme) that have passed fptestf.c

[snip]

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

CYGHWR_HAL_FPV4_SP_D16 also controls compilation of fpv4_sp_d16.c.

> 
> > Out of topic: 
[snip, thank you for explanation]

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

Mainly length i guess. But I accept your arguments, in that case it could be
either CYGBLD_ATTRIB_ALWAYS_INLINE (ATTRIB should go with ALWAYS to reflect
original attribute name) or (shorter) CYGBLD_FORCE_INLINE 

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.