Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CONFIG_FLOAT/CONFIG_FP_SHARING descriptions are confusing and contradictory #14122

Closed
pfalcon opened this issue Mar 6, 2019 · 15 comments
Closed
Assignees
Labels
area: Documentation bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug Waiting for response Waiting for author's response

Comments

@pfalcon
Copy link
Contributor

pfalcon commented Mar 6, 2019

https://docs.zephyrproject.org/latest/reference/kconfig/CONFIG_FLOAT.html
https://docs.zephyrproject.org/latest/reference/kconfig/CONFIG_FP_SHARING.html

The first says:

By default, only a single thread may use the registers.

Disabling this option means that any thread that uses a floating point register will get a fatal exception.

Isn't these 2 sentences contradict each other?

Now comparing 1st and 2nd doc:

This option allows threads to use the floating point registers.

This option allows multiple threads to use the floating point registers.

So, one option allows threads to use FP registers, and another allows multiple threads to use FP registers. But "threads" are already "multiple threads".

cc: @dbkinder for the help with wording, but I guess @nashif or another core maintainer first should confirm how those options are supposed to work.

@pfalcon pfalcon added the bug The issue is a bug, or the PR is fixing a bug label Mar 6, 2019
@andrewboie
Copy link
Contributor

CONFIG_FLOAT enables the FPU, but only one thread max is allowed to use it as the floating point registers are ignored by the context switching code.

CONFIG_FP_SHARING implements logic in the context switching code to save/restore fpu registers when threads are swapped in and out.

@pfalcon
Copy link
Contributor Author

pfalcon commented Mar 6, 2019

@andrewboie: Thanks for clarification, I guess I can prepare Kconfig help update out od that then.

but only one thread max is allowed to use it as the floating point registers are ignored by the context switching code.

Well, perhaps you could clarify: is it the main thread is allowed to use FP, or any thread at all on the "first grab, now mine" basis?

@andrewboie
Copy link
Contributor

Well, perhaps you could clarify: is it the main thread is allowed to use FP, or any thread at all on the "first grab, now mine" basis?

It doesn't matter which thread. Only that just a single one can use it.

@andrewboie
Copy link
Contributor

Also, and I think this may be architecture-dependent, but if the calling convention treats all floating point registers as volatile, then multiple threads can use the FPU with CONFIG_FP_SHARING disabled if and only if they are all of cooperative priority.

@pfalcon
Copy link
Contributor Author

pfalcon commented Mar 6, 2019

Also, and I think this may be architecture-dependent ...

Thanks, that's surely not for docs, but nice to know that there people who know/remember/can share such details.

Related, what me got to this ticket is looking for a good test for float functionality. And I find it "interesting" the fact that none of samples/* has CONFIG_FLOAT enabled (while a number has float in the C code).

OMG, and now I finally got it, CONFIG_FLOAT enables hardware FP. Not the best name for such an option, I'd say.

@pfalcon
Copy link
Contributor Author

pfalcon commented Mar 6, 2019

And yet, for qemu_x86 CONFIG_FLOAT=n and yet using float is fatal, no "soft float" is magically kicking in (that's what I thought would happen):

Running test suite test_libm
===================================================================
starting test - test_float_ops
***** Floating point unit not enabled
Current thread ID = 0x00401160
eax: 0x00000000, ebx: 0x00400000, ecx: 0x00423028, edx: 0x00401220
esi: 0x00000000, edi: 0x000020ac, ebp: 0x00405fc0, esp: 0x00405fb8
eflags: 0x00000216 cs: 0x0008
call trace:
eip: 0x0000134f
     0x00003e95 (0x0)
     0x000020bc (0x400000)
Fatal fault in thread 0x00401160! Aborting.

@pfalcon
Copy link
Contributor Author

pfalcon commented Mar 6, 2019

While for qemu_cortex_m3 it's ok.

@pfalcon pfalcon assigned pfalcon and unassigned nashif Mar 7, 2019
@SebastianBoe
Copy link
Collaborator

I agree that "use the floating point registers" is not clearly enough stating that this enables the FPU.

Also, I agree that it should be explicitly stated, and not just implied, that NOT CONFIG_FLOAT enables soft float.

qemu_x86 crashing when 'float' is used and NOT CONFIG_FLOAT is a bug/unexpected.

@nashif nashif added the priority: low Low impact/importance bug label Mar 9, 2019
@ioannisg
Copy link
Member

The text for floating point has been re-worked recently, in-line with what @andrewboie is suggesting here. IMHO it is clear that CONFIG_FLOAT allows for un-shared FP registers mode, while additional FP_SHARING allows for shared FP registers mode.

@pfalcon could you please, check if you could close this bug? (Or at least turn this into an enhancement) :)

@ioannisg
Copy link
Member

@pfalcon could you re-visit the documentation and comment if this issue described here still applies?

@ioannisg
Copy link
Member

@pfalcon ping

@ioannisg ioannisg added the Waiting for response Waiting for author's response label Feb 25, 2020
@nashif
Copy link
Member

nashif commented Mar 11, 2020

no response.

@stephanosio
Copy link
Member

So, one option allows threads to use FP registers, and another allows multiple threads to use FP registers. But "threads" are already "multiple threads".

I think phrasing could be improved here to say something like:

This option allows threads to use the floating point registers in an exclusive manner.

This option allows multiple threads to concurrently use the floating point registers.

@pfalcon
Copy link
Contributor Author

pfalcon commented Apr 23, 2020

@stephanosio: That might clarify it indeed. Please see if anything like that fits the scope of your #24636 (well, now that you touch that area at all). (And if not, it's fully ok.)

@stephanosio
Copy link
Member

Addressed in #24636.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Documentation bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug Waiting for response Waiting for author's response
Projects
None yet
Development

No branches or pull requests

6 participants