-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
disable ConstProp of floats #113416
disable ConstProp of floats #113416
Conversation
r? @cjgillot (rustbot has picked a reviewer for you, use r? to override) |
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
This comment has been minimized.
This comment has been minimized.
063f53b
to
18d6290
Compare
Could you add tests for the issues? |
18d6290
to
44c6999
Compare
// This affects at least binary ops and casts, so just skip all rvalues. | ||
// LLVM has a less buggy apfloat and will take care of const-propagation. | ||
return ValueOrPlace::TOP; | ||
} |
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.
Maybe it is better to short-circuit in handle_rvalue
then. But it would really help if the docs said whether TOP or BOTTOM represents "could be any value, statically unknown"...
What kind of tests would be best suited here? ui tests asserting the values? Or mir-opt tests? The latter get blessed away so easily I don't dare rely on them for things like this... |
This comment has been minimized.
This comment has been minimized.
Please let me know where else I should put these tests. There is no "float" directory. |
We have |
This has nothing to do with consts though. It's about const propagation (the optimization), not CTFE. |
0e4d70b
to
02f1c3f
Compare
Ah, there is a const_prop folder. I'll put them there. |
This comment has been minimized.
This comment has been minimized.
02f1c3f
to
8a8ffe6
Compare
This comment has been minimized.
This comment has been minimized.
Looks like on CI we have an old LLVM where the bugs have not been fixed yet. Should I set |
Good catch.
Could this manifest as a case where a
|
Yes, the bugs remain unfixed in CTFE. We don't have an easy work-around for that case. I figured it'd be better to at least fix them for the cases where we can easily do that. |
8a8ffe6
to
7153840
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
Looks like some LLVM passes are still running? The tests pass locally with newer LLVM... |
Closing in favor of #113843, which properly fixes the issues. :) |
This works around issues such as #102403 and #113407. The issues still affect
const
evaluation, but at least they don't affect run-time code any more.