Utilize discriminant_value for more efficient deriving #26280
Conversation
The new code generated for deriving on enums looks something like this:
```rust
let __self0_vi = unsafe {
std::intrinsics::discriminant_value(&self) } as i32;
let __self1_vi = unsafe {
std::intrinsics::discriminant_value(&__arg1) } as i32;
let __self2_vi = unsafe {
std::intrinsics::discriminant_value(&__arg2) } as i32;
///
if __self0_vi == __self1_vi && __self0_vi == __self2_vi && ... {
match (...) {
(Variant1, Variant1, ...) => Body1
(Variant2, Variant2, ...) => Body2,
...
_ => ::core::intrinsics::unreachable()
}
}
else {
... // catch-all remainder can inspect above variant index values.
}
```
This helps massively for C-like enums since they will be compiled as a
single comparison giving observed speedups of up to 8x. For more complex
enums the speedup is more difficult to measure but it should not be
slower to generate code this way regardless.
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
Do you know if this improves compile times of rustc? |
|
@pcwalton I haven't tested the compile times so I can't say for certain. C-like enums generate a lot better IR which should help make both compiled programs faster as well as compiling that code faster. Other enums should get a less noticeable speedup as well. I might do some benchmarking on the compiler tomorrow to see if it makes a difference though. |
| @@ -1048,15 +1048,23 @@ impl<'a> MethodDef<'a> { | |||
| /// discriminant values. See issue #15523.) | |||
There was a problem hiding this comment.
I looks like this bug #15523 was fixed, so it's a good time to update this comment too I think. I assume using discr value is always correct now?
|
Does this affect codegen for |
|
I ran the same benchmark on an Option and I got a 1.1x speedup. So I'd guess it helps a little though it might very well vary depending on what type the Option contains. |
|
That's nice. |
|
@pcwalton I compiled rustc a few times with and without this PR but I got to much noise to be able to say anything. Does not look like it makes a noticeable difference though. EDIT: Meant to write that it does not look like it makes a noticeable difference. |
Replaced it with a comment mentioning the rationale for checking the discriminants first.
|
r? @huonw, I think you're more familiar with this than I |
|
@bors: r+ |
|
📌 Commit 34d5b54 has been approved by |
PR for #26128. Improves codegen in deriving by utilizing the discriminant_value intrinsic. I have a more detailed comment on the changes in a comment on the issue [here](#26128 (comment)) ### Old ``` running 7 tests test large_c_like ... bench: 2,694,129 ns/iter (+/- 5,170) test large_c_like_ord ... bench: 2,723,521 ns/iter (+/- 9,098) test test1_partial_eq ... bench: 2,439,317 ns/iter (+/- 2,135) test test1_partial_ord ... bench: 2,499,114 ns/iter (+/- 55,766) test test2_partial_eq ... bench: 3,562,815 ns/iter (+/- 45,590) test test2_partial_ord ... bench: 3,398,306 ns/iter (+/- 22,180) test test_match_success ... bench: 1,509,267 ns/iter (+/- 1,982) ``` ### New ``` running 7 tests test large_c_like ... bench: 286,509 ns/iter (+/- 474) test large_c_like_ord ... bench: 286,666 ns/iter (+/- 8,756) test test1_partial_eq ... bench: 286,584 ns/iter (+/- 2,789) test test1_partial_ord ... bench: 286,470 ns/iter (+/- 516) test test2_partial_eq ... bench: 2,228,997 ns/iter (+/- 34,191) test test2_partial_ord ... bench: 1,731,699 ns/iter (+/- 21,756) test test_match_success ... bench: 1,509,630 ns/iter (+[- 3,765) ``` [Benchmark](https://gist.github.com/Marwes/7c0b3468d0cae972a2b4)
PR for #26128.
Improves codegen in deriving by utilizing the discriminant_value intrinsic.
I have a more detailed comment on the changes in a comment on the issue here
Old
New
Benchmark