Skip to content
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

Missing optimization when transmuting enums #109958

Closed
Jarcho opened this issue Apr 5, 2023 · 5 comments · Fixed by #109993
Closed

Missing optimization when transmuting enums #109958

Jarcho opened this issue Apr 5, 2023 · 5 comments · Fixed by #109993
Labels
C-bug Category: This is a bug. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Jarcho
Copy link
Contributor

Jarcho commented Apr 5, 2023

Transmuting a fieldless enum to it's integer type causes the optimizer to lose track of it's possible values. Note that as casts are optimized correctly.

Example:

#[repr(u32)]
enum E { X = 0 }
fn f(x: E) {
    assert_eq!(unsafe { core::mem::transmute::<E, i32>(x) }, 0); // Not optimized out
    assert_eq!(x as i32, 0); // optimized out
}

cc Lokathor/bytemuck#175

@Jarcho Jarcho added I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 5, 2023
@nikic
Copy link
Contributor

nikic commented Apr 5, 2023

Godbolt: https://rust.godbolt.org/z/ExbzPGf7s

cc @scottmcm in case your scalar transmute changes are relevant.

Not an LLVM problem because the necessary information is never provided to LLVM.

@clubby789
Copy link
Contributor

assumer(range.end, BinOp::Ge);
assumer(range.start, BinOp::Le);

We emit a couple of assume's when casting an enum with as but no extra info is emitted for a transmute

@Nilstrieb Nilstrieb added the C-bug Category: This is a bug. label Apr 5, 2023
@erikdesjardins
Copy link
Contributor

If we could just put range metadata on arguments, we could emit

define noundef i32 @f(i32 noundef range(!0) %x)
...
!0 = !{i32 0, i32 1}

and then this and other cases (e.g. NonZeroX::get) would work without us having to add assumes for every conversion.

I actually have a partial implementation of this here. Though I assume allowing attributes to depend on metadata nodes would be untenable, they're probably separate intentionally? (@nikic thoughts?)

(My implementation also doesn't fully work because of this problem with RAUW on forward-declared metadata: erikdesjardins/llvm-project@c1342ac#diff-100c1b4a714c7d260fc41ac27cc8d2d5857d54021f16d73debc71125d110e7daR2809-R2831)

The alternative would be something like range(0, 1, 4, 5), where it doesn't use metadata to represent the ranges, and stores it in a bespoke attribute type, but I think this would also be a bit awkward. All range metadata handling code depends on the metadata representation (this could be refactored for sure), and attributes aren't equipped to hold variable-size arguments except strings, so I think some significant changes to Attribute would be required.

(Of course the other alternative would be to just add the assumes, but it feels a bit unsatisfying)

@scottmcm
Copy link
Member

scottmcm commented Apr 5, 2023

Transmuting a fieldless enum to it's integer type causes the optimizer to lose track of it's possible values.

If you do this directly to the repr type, it should no longer be a problem, since we MIR-optimize a transmute like that to a discriminant read: https://github.com/rust-lang/rust/pull/109612/files#diff-4f93c014037e17032815fe49c647952b4faf26aff2007afd9a36106f8d0f39f6R86-R87.

Hmm, playground is only on the 2023-04-02 nightly, so I can't look there to see if my change improved things.

@scottmcm
Copy link
Member

scottmcm commented Apr 5, 2023

With #109843, this:

// CHECK-LABEL: @check_to_enum(
#[no_mangle]
pub unsafe fn check_to_enum(x: i8) -> SmallEnum {
    transmute(x)
}

// CHECK-LABEL: @check_from_enum(
#[no_mangle]
pub unsafe fn check_from_enum(x: SmallEnum) -> i8 {
    transmute(x)
}

just emits

; Function Attrs: uwtable
define noundef i8 @check_to_enum(i8 noundef %x) unnamed_addr #0 {
start:
  ret i8 %x
}

; Function Attrs: uwtable
define noundef i8 @check_from_enum(i8 noundef %x) unnamed_addr #0 {
start:
  ret i8 %x
}

since it's passed as an immediate.

Loading an enum puts !range metadata on the load, but since there's no load here the OperandValue stuff I'm doing in the new code doesn't do anything with the niches.

bors added a commit to rust-lang-ci/rust that referenced this issue Apr 20, 2023
@bors bors closed this as completed in 1bcb0ec Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants