-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Fix FP env issues #11087
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
Fix FP env issues #11087
Conversation
…f out pragmas we don't support anyhow
|
This makes libc 164 bytes smaller. |
|
I think some change to the fenv defs might make sense (although, should we do something similar for rounding modes?), but I'm not so thrilled about ifdef-ing out the pragmas for the long term. I think we will still want to handle the constrained intrinsics in one form or another. Perhaps with an ISel fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to work. I still predict that if we don't fix this for real soon, somebody will complain about the regression. Or maybe theyll just comment out their pragmas, who knows 🤷
| #define FE_DIVBYZERO 4 | ||
| #define FE_OVERFLOW 8 | ||
| #define FE_UNDERFLOW 16 | ||
| #define FE_INEXACT 32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these designed to be disabled by simply not being defined? That seems unusual, but I don't know this part of the standard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I learned this today as well. It is quite unusual for the C spec, I agree.
Libraries may define in <fenv.h> only the macro values above they support (the others may not be defined).
http://www.cplusplus.com/reference/cfenv/FE_INEXACT/
And musl in fact checks whether things are defined in order to ifdef relevant code. Which actually is helpful in this case!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if they are designed that way exactly, but musl code seems to use them that way: see e.g. https://github.com/emscripten-core/emscripten/blob/master/system/lib/libc/musl/src/fenv/__flt_rounds.c and in particular https://github.com/emscripten-core/emscripten/blob/2bca083cbbd5a4133db61fbd74d04f7feecfa907/system/lib/libc/musl/src/math/lrint.c gates its use of the FENV_ACCESS pragma on FE_INEXACT being defined.
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
Intentionally missing per emscripten-core/emscripten#11087
avoids a lot of float pragmas we don't support.
Helps #11086 , but does not fix it as user code may still be broken
due to LLVM failing ISel on those pragmas now.