Skip to content

Tell LLVM about impossible niche tags #139098

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 8, 2025
Merged

Tell LLVM about impossible niche tags #139098

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f06a6a
Tell LLVM about impossible niche tags
scottmcm Mar 29, 2025
51e67e2
LLVM18 compatibility fixes in the tests
scottmcm Mar 29, 2025
502f7f9
Address PR feedback
scottmcm Apr 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Address PR feedback
  • Loading branch information
@scottmcm
scottmcm committed Apr 8, 2025
commit 502f7f9c241b35d65f4538d28c43e6feca474df2
4 changes: 3 additions & 1 deletion compiler/rustc_codegen_ssa/src/mir/operand.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,7 +581,9 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {
);

// In principle we could insert assumes on the possible range of `discr`, but
// currently in LLVM this seems to be a pessimization.
// currently in LLVM this isn't worth it because the original `tag` will
// have either a `range` parameter attribute or `!range` metadata,
// or come from a `transmute` that already `assume`d it.

discr
}
Expand Down
16 changes: 16 additions & 0 deletions tests/codegen/enum/enum-two-variants-match.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@

#![crate_type = "lib"]

// This directly tests what we emit for these matches, rather than what happens
// after optimization, so it doesn't need to worry about extra flags on the
// instructions and is less susceptible to being broken on LLVM updates.

// CHECK-LABEL: @option_match
#[no_mangle]
pub fn option_match(x: Option<i32>) -> u16 {
Expand Down Expand Up @@ -103,10 +107,22 @@ pub fn option_ordering_match(x: Option<Ordering>) -> char {
// CHECK-LABEL: @option_nonzero_match(
#[no_mangle]
pub fn option_nonzero_match(x: Option<std::num::NonZero<u16>>) -> u16 {
// CHECK: %[[OUT:.+]] = alloca [2 x i8]

// CHECK: %[[IS_NONE:.+]] = icmp eq i16 %x, 0
// CHECK: %[[OPT_DISCR:.+]] = select i1 %[[IS_NONE]], i64 0, i64 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to not emit a select if it's equivalent to zext? Or would that be pointless?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if it's really worth it vs just letting LLVM do it for us.

(And this one would need to flip the icmp eq to icmp ne in order to use a zext, which I think is actually always the case for a niche-encoded Option. So maybe it'd be worth flipping the way we emit things so that we could then use the zext, but it's not something I'd bother doing in this PR. Really the most silly part is that we need to widen to isize only to trunc down to bool anyway, but those come from different parts of the MIR so it's hard to fix without adding new MIR things that probably aren't worth it, so I'm inclined to just stick with the "meh, LLVM will handle it" approach.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I originally thought about widen/trunc too, but figured that this is just multiple parts of our code assuming the type.

// CHECK: %[[OPT_DISCR_T:.+]] = trunc nuw i64 %[[OPT_DISCR]] to i1
// CHECK: br i1 %[[OPT_DISCR_T]], label %[[BB_SOME:.+]], label %[[BB_NONE:.+]]

// CHECK: [[BB_SOME]]:
// CHECK: store i16 987, ptr %[[OUT]]

// CHECK: [[BB_NONE]]:
// CHECK: store i16 123, ptr %[[OUT]]

// CHECK: %[[RET:.+]] = load i16, ptr %[[OUT]]
// CHECK: ret i16 %[[RET]]

match x {
None => 123,
Some(_) => 987,
Expand Down
Loading