Use #[rustc_inherit_overflow_checks] instead of Add::add etc.
#81732
Conversation
m-ou-se
added
the
S-experimental
This comment has been minimized.
This comment has been minimized.
m-ou-se
force-pushed
the
inherit-overflow-checks
branch
from
96cc6e5
to
721e9b7
Compare
This comment has been minimized.
This comment has been minimized.
m-ou-se
force-pushed
the
inherit-overflow-checks
branch
from
721e9b7
to
bd1eb21
Compare
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit bd1eb210f1636b4e69055bc37b1a5b118a2fed26 with merge 373cca1b4f95bdf0298d4add9943f805161c8c87... |
| #[rustc_inherit_overflow_checks] | ||
| |a, b| a + b, |
There was a problem hiding this comment.
Hah, does this work? I half-expected it not to.
There was a problem hiding this comment.
Haha, this was your suggestion.. But yeah, it compiles. I think it works too, but I should check if it actually gets tested for this behaviour. :)
There was a problem hiding this comment.
Yeah, I was going off having seen #[inline(never)], IIRC, on closures, but I wasn't sure it would work out of the box.
|
☀️ Try build successful - checks-actions |
|
Queued 373cca1b4f95bdf0298d4add9943f805161c8c87 with parent e708cbd, future comparison URL. |
|
Finished benchmarking try commit (373cca1b4f95bdf0298d4add9943f805161c8c87): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
rustbot
added
S-waiting-on-review
|
Okay this looks good, no regression in performance, and even a few percent improvement for cranelift. To do: Verify that it actually works the same as before. I'm not sure if we test all these cases for ther release/debug-mode panic behaviour. |
m-ou-se
added
the
T-libs
m-ou-se
force-pushed
the
inherit-overflow-checks
branch
from
bd1eb21
to
053769d
Compare
Manually verified that it behaves the same. Looks like there's also already tests for these in |
m-ou-se
removed
the
S-experimental
| iter.fold($zero, Add::add) | ||
| iter.fold( | ||
| $zero, | ||
| #[rustc_inherit_overflow_checks] |
There was a problem hiding this comment.
Hm, so I initially thought "this seems wrong, don't we need it on the fn sum too? But it looks like in practice inherit is a bit wrong - this attribute just forces the MIR to contain overflow checks in all cases, and then when we codegen (potentially in a downstream crate) they'll get removed. So I think we're good -- not sure what a better name for the attribute is, but inherit definitely feels a bit off.
There was a problem hiding this comment.
Yeah, the inherit here is about crates rather than functions, in some sense.
There was a problem hiding this comment.
Yeah, "inherit" refers to the crate it's codegen'd in. So it's more "defer"? Or we can go more explicit really:
#[rustc_use_overflow_checks_settings_from_user_crate]
We could also make the attribute warn/error if building the MIR doesn't end up making a decision based on it, because that's what usually happens if it's applied to the wrong thing (e.g. a function doing iter.fold(...) instead of a closure).
|
@bors r+ |
|
📌 Commit 053769d has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
See #81721