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 web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607

--- Comment #39 from Jonathan Larmour <jifl@ecoscentric.com> 2012-12-13 05:37:23 GMT ---
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.

(In reply to comment #24)
> Here are some comments. In the text below I am referencing with your numbers
> from Comment #16 and Comment #17.
> 
> 1) Cortex-M4/M3 selector is removed, and CYGBLD_ARCH_XFLAGS added, with comment
> regarding GCC issue. Anti-flags (4) moved to CYGINT_HAL_CORTEXM_FPU and
> CYGINT_HAL_FPV4_SP_D16 (respecting FPU abstraction). There is, however a
> problem with unresolved conflicts when I click on CYGHWR_HAL_CORTEXM_FPU.
> Funny, but it resolves when I right-click RESOLVE in conflict window.

This seems to be okay in your latest version of the patch.

> 19) Your reg_offset() code works. It would be inappropriate for me to sign it,
> so I would be happy to see you in the list of contributors.
>
> Also hal_cortex_fpu.cdl is at least 40% your ideas so we would share the
> authorship.

Thanks. Just be sure to put your name first in the changelog though because
you've definitely done the hard work!

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

(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;
but also simply because it's more important to be safe by default. People will
always be able to disable it.


(In reply to comment #27)
> There's however one essential addition: option for softfp ABI. Being able to
> link softfp code with -mfloat-abi=soft, the -mfloat-abi=softfp flag (when
> selected) is applied only to context.S and vectors.S. All other code compiles
> with -mfloat-abi=soft thus avoiding potential GCC's "optimizations" - see Note
> below.
>
> Note (RFC): according to
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dai0298a/DAFIFGGE.html
> if hardware floating point option is selected, GCC may choose to use FPU
> registers even if there isn't floating point source. Please comment.
[and also the gcc-help link]

That's inconvenient. I think because of this it might be wise to always
explicitly use either -mfloat-abi=soft or -mfloat-abi=hard. So as well as
removing -mfloat-abi=hard in the current set of requires at the bottom of
hal_cortexm.cdl, we would also force in -mfloat-abi=soft (or -msoft-float). I
don't see a need for softfp - I think that's mostly aimed at people requiring
to interwork with prebuilt binaries.

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)
> Therefore I decided to move all operations in a single CDL that is
> CYGBLD_ARCH_CPUFLAGS. Besides working smoothly it also solves the following
> problems: [snip]

I like this. Sneaky but effective!

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

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

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

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.

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.


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