[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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1784|0                           |1
        is obsolete|                            |

--- Comment #13 from Ilija Kocho <ilijak@siva.com.mk> 2012-07-08 17:50:29 BST ---
Created an attachment (id=1816)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1816)
Cortex-M4F Floating Point Support 120708

Hi Jifl

Here I post updated FPU patch.
Below are replies to some of your comments.

(In reply to comment #12)
> Hi Ilija,
> 
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Architecture HAL:
> > > 
> > > - The CDL seems to be getting much more complex than it really needs to be.
> > > There is now CDL for: CYGHWR_HAL_CORTEXM (== M3 or M4),
> > > CYGINT_HAL_CORTEXM4_CODE, CYGOPT_HAL_CORTEXM4_CODE, CYGOPT_HAL_CORTEXM3_CODE,
> > > CYGINT_HAL_CORTEXM_FPU, CYGINT_HAL_CORTEXM_FPV4_SP_D16, CYGHWR_HAL_CORTEXM_FPU,
> > > and to an extent CYGHWR_HAL_CORTEXM_FPU_SWITCH=="NONE", which all have
> > > overlapping behaviour with respect to each other and effects on each other.
> > > There is unnecessary redundancy here.
> > > 
> > > I think we can simply remove CYGINT_HAL_CORTEXM4_CODE, CYGOPT_HAL_CORTEXM4_CODE
> > > and CYGOPT_HAL_CORTEXM3_CODE because even if building with -mcpu=cortex-m4, gcc
> > > defaults to soft floating-point. So that means if CYGHWR_HAL_CORTEXM == "M4"
> > > then it's safe to use -mcpu=cortex-m4. So CYGBLD_GLOBAL_XFLAGS_ARCH can go too.
> > 
> > I am aware of this excessive complexity, as it gave me some headache but I'm
> > afraid that for the time being it is necesary. The reason for keeping
> > -mcpu=cortex-m3 option even for Cortex-M4 cores, is compatibility with the
> > actual eCos compiler, gcc-4.3.2, that doesn't support/recognize
> > -mcpu=cortex-m4. We can reconsider once we switch to gcc-4.6.3 or newer. 
> 
> Okay. But can't we just let the user do this by changing the value of
> CYGHWR_HAL_CORTEXM? We can just (temporarily) relax the CYGHWR_HAL_CORTEXM ==
> "M4" in the kinetis variant HAL. Since we can't use M4 features without the
> compiler support, in practice it's treating as an M3 anyway. Then we ensure
> CFLAGS/LDFLAGS get set according to CYGHWR_HAL_CORTEXM.
> 

I think that this work-arround would just move the problem from one place to
other. Now I have placed radio buttons and it (at least) looks nicer.


> > > - Here's a possible outline of a simplification. What do you think?
> > >   cdl_option CYGHWR_HAL_CORTEXM_FPU_FLAGS {

I made some changes to this CDL. I think we're bit closer.

> > 
> > > - There is a quite important design issue. A lot of things assume the only
> > > processor with an FPU is the Cortex-M4 and that FPU has the fpv4-sp-d16
> > > arrangement. This is unlikely to remain the case, and it's easy to anticipate.
> > > The M4 FPU is a bit cut-down after all, so it's easy to see a full VFP4 unit
> > > being added in a later Cortex-M.  So we should be wrapping up much more
> > > functionality in macros that can be supplied according to the FPU design, the
> > > most obvious alternative possibility being a full VFP4 unit, to give an idea of
> > > what to abstract.
> > > 
> > > With that in mind, I think there needs to be a fpv4-sp-d16.h header file,
> > > probably included from cortexm_fpu.h (which could in future then instead
> > > include alternatives e.g. a vfp4.h header once that became relevant). That
> > > FPU-specific header will define a number of FPU-model-specific properties and
> > > macros used for the implementation, such as from cortexm_stub.h and context.S. 
> > > 
> > 

I wrote macros for vectors.S but left context.S without macros. The reason
vectors.S functions are easily represented as sequences of macros while for
context.S it would either give less optimal code.

Also introduced fpv4_sp_d16.[h|c]

> 
> 
> Although thinking about it, it probably does want a CYG_MACRO_START/END
> wrapper, otherwise (although very unlikely) someone could theoretically do:
> if (something)
>   HAL_MEMORY_BARRIER();
> and things might not behave as intended.

I forgot this, sorry. Now I'm posting as is but, I will add the wrapper in next
iteration.

> > > - You don't save the floating point context for interrupts, but do for
> > > exceptions. There's an argument that since both interrupt and exception
> > > handlers could call arbitrary code which therefore may use FP, the context
> > > should be saved. There's also an argument that unless this is a ROM monitor and
> > > including stubs, you don't need to save the exception info either. I think
> > > there may be merit in having two config options to govern whether to save FP
> > > state for exceptions and interrupts. The default value for the exceptions one
> > > would be CYGSEM_HAL_ROM_MONITOR || CYGDBG_HAL_DEBUG_GDB_INCLUDE_STUBS, the
> > > default for the interrupt one would be 0.
> > 
> > I expected some arguments here... I can't imagine myself using FPU in ISR or
> > DSR, but if it is an option, disabled by default here are some considerations:
> >   - ALL: Implementation is easy, we just skip disabling of automatic stack
> > saving.
> >   - LAZY: Lazy saving introduces variable frame length that requires handling
> > that will produce worst case latency even worse than for ALL. It requires suble
> > intervention in ISR and may unnecessarily delay the project.
> > 
> > Therefore I would consider immediate implementation for ALL, but for LAZY i
> > would prefer to put "Not implemented in current release" for the time being.
> 
> I think that's ok. So, to be clear, there would be options for whether to save
> FP state for interrupts and exceptions, and for the interrupt one, it would
> require !LAZY?

Added automatic context saving option for ALL. Need's test programs.

> > 
> > The automatic stack frame is different than the one used in
> > default_exception_vsr
> 
> Of course, yes, that slipped my mind.
> 
> > > - hal_usagefault_exception_vsr is almost identical to
> > > hal_default_exception_vsr. I don't see why hal_default_exception_vsr can't be
> > > used, and instead treated specially in hal_deliver_exception() instead, which
> > > would be less code overall.
> > > 
> > 
> > Cortex-M architecture, unlike pure ARM, provides us with individual
> > exception/interrupt vectors and it would be waste not to use them. We get both
> > simpler, cleaner and faster code. I would rather extend this approach to other
> > other exception handlers.
> 
> For interrupts I might agree, but exceptions are as defined in the name -
> exceptional, i.e. unusual. They aren't common so runtime performance is not the
> same concern. Duplicating each one is just a waste of code space. The
> efficiency saving is probably only about 3 instructions (a mov, comparison and
> branch). And this for something which only ever really gets used in lazy FPU
> mode, and then only if a thread uses FP, and then only once ever per thread.
> 
> Also as I mentioned, it also breaks the ability to properly debug an unhandled
> usage fault with the stub.
> 

Please note that when using FPU the hal_usagefault_exception_vsr and
hal_default_exception_vsr have different prologues/epilogues.


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.