Also assume wrap-around discriminants in as MIR building

[scottmcm]

Resolves this FIXME:

rust/compiler/rustc_mir_build/src/build/expr/as_rvalue.rs

Line 231 in 8d18c32

// FIXME: Handle wraparound cases too.

r? @oli-obk

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[scottmcm]

Without the library change, this was giving

define noundef zeroext i1 @ordering_eq(i8 noundef %0, i8 noundef %1) unnamed_addr #0 personality ptr @__CxxFrameHandler3 {
start:
  %2 = icmp eq i8 %0, 2
  br i1 %2, label %"_ZN4core6option15Option$LT$T$GT$6map_or17h10b30776faeff97fE.exit", label %bb3.i

bb3.i:                                            ; preds = %start
  %3 = add i8 %0, 1
  %_7.i.i = icmp ult i8 %3, 3
  tail call void @llvm.assume(i1 %_7.i.i)
  br label %"_ZN4core6option15Option$LT$T$GT$6map_or17h10b30776faeff97fE.exit"

"_ZN4core6option15Option$LT$T$GT$6map_or17h10b30776faeff97fE.exit": ; preds = %start, %bb3.i
  %4 = icmp eq i8 %1, 2
  br i1 %4, label %"_ZN4core6option15Option$LT$T$GT$6map_or17hd4add697368b47d3E.exit", label %bb3.i2

bb3.i2:                                           ; preds = %"_ZN4core6option15Option$LT$T$GT$6map_or17h10b30776faeff97fE.exit"
  %5 = add i8 %1, 1
  %_7.i.i1 = icmp ult i8 %5, 3
  tail call void @llvm.assume(i1 %_7.i.i1)
  br label %"_ZN4core6option15Option$LT$T$GT$6map_or17hd4add697368b47d3E.exit"

"_ZN4core6option15Option$LT$T$GT$6map_or17hd4add697368b47d3E.exit": ; preds = %"_ZN4core6option15Option$LT$T$GT$6map_or17h10b30776faeff97fE.exit", %bb3.i2
  %6 = icmp eq i8 %0, %1
  ret i1 %6
}

Now that it's using the transmute instead of the map_or approach, it's

define noundef zeroext i1 @ordering_eq(i8 noundef %0, i8 noundef %1) unnamed_addr #0 {
start:
  %2 = add i8 %0, 1
  %3 = icmp ult i8 %2, 4
  tail call void @llvm.assume(i1 %3)
  %4 = add i8 %1, 1
  %5 = icmp ult i8 %4, 4
  tail call void @llvm.assume(i1 %5)
  %6 = icmp eq i8 %0, %1
  ret i1 %6
}

[oli-obk]

While we can do this hack in libcore, this means user code will regress in similar situations. Should we track this issue to figure out how to improve it? We could autogenerate the transmute in a MIR opt for example.

[scottmcm]

I don't think it should block this PR, because it's only the fluke of its chosen discriminants -- using more normal enums the map_or plan already didn't work: https://rust.godbolt.org/z/Tnvvev789

I could file a Rust issue, but I think the "right" fix would be fixing the root issue that made us start using this mitigation in the first place: Having LLVM just handle the derived equality better llvm/llvm-project#52622

(This is a hack inside a specialization hack, so I don't know that it's necessarily representative of user code.)

[scottmcm]

Hmm, actually, maybe it's not a real problem. It looks bad in the LLVM IR, but the assembly generated for it is still the correct single test: https://rust.godbolt.org/z/da819G99e

example::qux:
        cmp     dil, sil
        sete    al
        ret

despite

define noundef zeroext i1 @_ZN7example3qux17hb5cd12f675ddd331E(i8 noundef %x, i8 noundef %y) unnamed_addr #0 personality ptr @rust_eh_personality {
  %0 = icmp eq i8 %x, -128
  br i1 %0, label %"_ZN4core6option15Option$LT$T$GT$6map_or17h8cb56c5fe57f6f5dE.exit", label %bb3.i

bb3.i:                                            ; preds = %start
  %_4.i.i = icmp sgt i8 %x, -1
  tail call void @llvm.assume(i1 %_4.i.i)
  br label %"_ZN4core6option15Option$LT$T$GT$6map_or17h8cb56c5fe57f6f5dE.exit"

"_ZN4core6option15Option$LT$T$GT$6map_or17h8cb56c5fe57f6f5dE.exit": ; preds = %start, %bb3.i
  %1 = icmp eq i8 %y, -128
  br i1 %1, label %"_ZN4core6option15Option$LT$T$GT$6map_or17h93b3ae047b8aab61E.exit", label %bb3.i2

bb3.i2:                                           ; preds = %"_ZN4core6option15Option$LT$T$GT$6map_or17h8cb56c5fe57f6f5dE.exit"
  %_4.i.i1 = icmp sgt i8 %y, -1
  tail call void @llvm.assume(i1 %_4.i.i1)
  br label %"_ZN4core6option15Option$LT$T$GT$6map_or17h93b3ae047b8aab61E.exit"

"_ZN4core6option15Option$LT$T$GT$6map_or17h93b3ae047b8aab61E.exit": ; preds = %"_ZN4core6option15Option$LT$T$GT$6map_or17h8cb56c5fe57f6f5dE.exit", %bb3.i2
  %2 = icmp eq i8 %x, %y
  ret i1 %2
}

So I suppose I could also try moving these tests to assembly tests if that would be better, and leaving the map_ors.

[scottmcm]

Based on the thumb, I'll do that.

@rustbot author

[scottmcm]

Ok, removed the library/ change, and instead moved the test to assembly. Didn't change the compiler logic.

@rustbot ready

[oli-obk]

@bors r+

[bors]

📌 Commit 400866b has been approved by oli-obk

It is now in the queue for this repository.