-
Notifications
You must be signed in to change notification settings - Fork 14k
Always inline functions signatures containing f16 or f128
#133050
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| //@ revisions: default nopt | ||
| //@[nopt] compile-flags: -Copt-level=0 -Zcross-crate-inline-threshold=never -Zmir-opt-level=0 -Cno-prepopulate-passes | ||
|
|
||
| // Ensure that functions using `f16` and `f128` are always inlined to avoid crashes | ||
| // when the backend does not support these types. | ||
|
|
||
| #![crate_type = "lib"] | ||
| #![feature(f128)] | ||
| #![feature(f16)] | ||
|
|
||
| pub fn f16_arg(_a: f16) { | ||
| // CHECK-NOT: f16_arg | ||
| todo!() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure the automatic inlining heuristics wouldn't anyway already mark these functions as inlineable? Might be worth setting cross_crate_inline_threshold to 0 to be sure that doesn't mask the test.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a revision with
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should also add
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yeah of course. Done, tests still pass.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
|
|
||
| pub fn f16_ret() -> f16 { | ||
| // CHECK-NOT: f16_ret | ||
| todo!() | ||
| } | ||
|
|
||
| pub fn f128_arg(_a: f128) { | ||
| // CHECK-NOT: f128_arg | ||
| todo!() | ||
| } | ||
|
|
||
| pub fn f128_ret() -> f128 { | ||
| // CHECK-NOT: f128_ret | ||
| todo!() | ||
| } | ||
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.
if you move this down after
let mir, you can just iterate overmir.args_iter()instead of having to callfn_sigThere 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.
I think this would require moving
let mirabove the checks againstopts.incremental,OptLevel::No, andthreasholdto avoid hitting areturn falsebefore we check for the types. Would this wind up with a perf impact since we don't generate mir here in these cases?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.
Hm, I thought having this check so far down would be fine. But maybe not.
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.
Since this is a (temporary, right?) hack, I'd rather the diff be small and localized than pretty. What you've written here is good.
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.
Yes please, I don't think we will be able to stabilize these types until LLVM at least doesn't crash on everything T2+.