This is the mail archive of the ecos-patches@sourceware.org mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[Bug 1001607] Cortex-M4F architectural Floating Point Support


Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

--- Comment #52 from Jonathan Larmour <jifl@ecoscentric.com> ---
Hi Ilija,

(In reply to comment #49)
That's great that the lazy stacking will work after all.

In light of this, I am going to assume that comment #42 and comment #47 are now
irrelevant and can be ignored entirely. Let me know if not. Some other
responses:

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

But then we would lose that optimisation, which is a shame. However I think
with the new code using CPU lazy stacking for exceptions and the removal of the
EXC_AUTOSAVE CDL option, all we would need in future if/when that gcc
optimisation happens, is to add -fno-fpreg-for-nonfpdata to CFLAGS if
(CYGINT_HAL_CORTEXM_FPU && !CYGHWR_HAL_CORTEXM_FPU). This also assumes by that
point we are also using -mcpu=cortex-m4 always on the M4 (uncommenting the
relevant line in the CDL). But for now we do nothing.

> >  - 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 had a look and I think anything from GCC 4.5 may at some point optimise this
(even if you haven't had it happen yet):
http://gcc.gnu.org/gcc-4.5/changes.html talks about the use of the MPC library
under "General Optimizer Improvements", third bullet point. But yes, I now see
you use rand() which will nicely solve the problem :).

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

Yes of course you're right.

(In reply to comment #45)
>     - fpinttest.c renamed fpinttestf.c.
>     - Added fpinttestf1.c for testing of NONE context switching scheme, as
> it normally fails fpinttestf.c

In that case fpinttestf.c should use CYG_TEST_NA if it's using the NONE context
scheme - tests should do the right thing whatever the configuration.

So I suggest adding this to the tests in fpinttestf.c for when to be NA
(including in the CYG_TEST_NA call itself at the bottom):
 (!defined(CYGHWR_HAL_CORTEXM_FPU) ||
!defined(CYGHWR_HAL_CORTEXM_FPU_SWITCH_NONE))


(In reply to comment #50)
> 
> Here is a patch with CYGARC_CORTEXM_FPU_EXC_AUTOSAVE option removed.
> CYGARC_CORTEXM_FPU_EXC_AUTOSAVE still lives as a macro defined in
> fpv4_sp_d16.h.

Hmm, so I've now started wondering about saving the context in lazy mode as
well. You already set CYGARC_REG_FPU_FPCCR_ASPEN/LSPEN for lazy mode which
means the CPU will already be doing lazy stacking in interrupt/exception
handlers.

So in fact, can we just change the define at the top of fpv4_sp_d16.h to:

#if defined CYGHWR_HAL_CORTEXM_FPU_SWITCH_ALL || \
    defined CYGHWR_HAL_CORTEXM_FPU_SWITCH_LAZY
#define CYGARC_CORTEXM_FPU_EXC_AUTOSAVE
#endif

and then change the CYGHWR_HAL_CORTEXM_FPU_SWITCH_ALL test on line 158 to also
include LAZY, and then it might just work for lazy mode too? I can't see
anything else in the way.

> Tests, included in this patch are general. The only Cortex-M depndent code
> is HAL_CLOCK_READ_NS() that can be conditionaly replaced with
> HAL_CLOCK_READ() for other architectures. Then we can consider placing
> these tests in kernel or hal/common.

In principle, fpinttestf1 could run from cyg_start with no dependency on the
kernel, however that would require a few changes. But if you did change that,
then all the fp* tests may as well go in hal/common. Otherwise, if you don't
have time to adjust the tests, then since they all use kernel threads at
present then they should all just go in the kernel.

Hmm, out of interest, one of the contributions I intend to make when I have
time(!) is a complete revamp of the HAL_CLOCK* API with a lot more control and
the ability to convert to/from nsecs. Not today though...

> Speaking of HAL_CLOCK_READ_NS(), is it desirable for inclusion in cortex-m
> headers?

Despite my upcoming contribution, you can go ahead and add this to Cortex-M -
it's harmless even if it isn't the future :-). Obviously the tests still need
some changes to fall back on HAL_CLOCK_READ() still.

> And to continue with our off topic: I would go with CYGBLD_FORCE_INLINE
> if you don't object, what is your opinion?

CYGBLD_FORCE_INLINE or CYGBLD_ALWAYS_INLINE, either is fine with me.

A couple of other little things:

For the ChangeLog, to comply with the GNU standards, the format should really
be this:
2012-04-23  Ilija Kocho  <ilijak@siva.com.mk>
2012-04-23  Jonathan Larmour  <jifl@eCosCentric.com>

i.e. two consecutive lines. Still your name first obviously.

BTW don't forget at some point to either tidy up the indentations in the CDL,
or let me know and I will do so after check-in - you've done enough after all!

Thanks for all this,

Jifl

-- 
You are receiving this mail because:
You are on the CC list for the bug.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]