Fix repr(align) enum handling #96814

RalfJung · 2022-05-07T13:08:01Z

enum, for better or worse, supports repr(align). That has already caused a bug in #92464, which was "fixed" in #92932, but it turns out that that fix is wrong and caused #96185.

So this reverts #92932 (which fixes #96185), and attempts another strategy for fixing #92464: special-case enums when doing a cast, re-using the code to load the discriminant rather than assuming that the enum has scalar layout. This works fine for the interpreter.

However, #92464 contained another testcase that was previously not in the test suite -- and after adding it, it ICEs again. This is not surprising; codegen needs the same patch that I did in the interpreter. Probably this has to happen around here. Unfortunately I don't know how to do that -- the interpreter can load a discriminant from an operand, but codegen can only do that from a place. @oli-obk @eddyb @bjorn3 any idea?

rust-highfive · 2022-05-07T13:08:04Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

rust-highfive · 2022-05-07T13:08:06Z

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

eddyb

r=me with comment fix (just the typo would be enough, but I got side-tracked using the change suggestion UI and added some more details)

compiler/rustc_middle/src/ty/layout.rs

compiler/rustc_const_eval/src/interpret/cast.rs

eddyb · 2022-05-07T17:26:17Z

However, #92464 contained another testcase that was previously not in the test suite -- and after adding it, it ICEs again. This is not surprising; codegen needs the same patch that I did in the interpreter. Probably this has to happen around here. Unfortunately I don't know how to do that -- the interpreter can load a discriminant from an operand, but codegen can only do that from a place. @oli-obk @eddyb @bjorn3 any idea?

Ah I missed this, right, so this PR not ready for merging.
There's also this very interesting piece of code I noticed while looking around codegen:

rust/compiler/rustc_codegen_ssa/src/mir/rvalue.rs

Lines 301 to 340 in d32ce37

    
           if let Abi::Scalar(scalar) = operand.layout.abi { 
        
               if let Int(_, s) = scalar.primitive() { 
        
                   // We use `i1` for bytes that are always `0` or `1`, 
        
                   // e.g., `#[repr(i8)] enum E { A, B }`, but we can't 
        
                   // let LLVM interpret the `i1` as signed, because 
        
                   // then `i1 1` (i.e., E::B) is effectively `i8 -1`. 
        
                   signed = !scalar.is_bool() && s; 
        
                   if !scalar.is_always_valid(bx.cx()) 
        
                       && scalar.valid_range(bx.cx()).end 
        
                           >= scalar.valid_range(bx.cx()).start 
        
                   { 
        
                       // We want `table[e as usize ± k]` to not 
        
                       // have bound checks, and this is the most 
        
                       // convenient place to put the `assume`s. 
        
                       if scalar.valid_range(bx.cx()).start > 0 { 
        
                           let enum_value_lower_bound = bx.cx().const_uint_big( 
        
                               ll_t_in, 
        
                               scalar.valid_range(bx.cx()).start, 
        
                           ); 
        
                           let cmp_start = bx.icmp( 
        
                               IntPredicate::IntUGE, 
        
                               llval, 
        
                               enum_value_lower_bound, 
        
                           ); 
        
                           bx.assume(cmp_start); 
        
                       } 
        
                       let enum_value_upper_bound = bx 
        
                           .cx() 
        
                           .const_uint_big(ll_t_in, scalar.valid_range(bx.cx()).end); 
        
                       let cmp_end = bx.icmp( 
        
                           IntPredicate::IntULE, 
        
                           llval, 
        
                           enum_value_upper_bound, 
        
                       ); 
        
                       bx.assume(cmp_end); 
        
                   } 
        
               } 
        
           }

So the discriminant reading should happen before that, with let llval = operand.immediate(); presumably special-cased when operand.layout.ty.is_enum().
The llval value just before the actual int2int cast would be identical to this I'm guessing:

rust/compiler/rustc_codegen_ssa/src/mir/rvalue.rs

Lines 475 to 488 in d32ce37

    
           mir::Rvalue::Discriminant(ref place) => { 
        
               let discr_ty = rvalue.ty(self.mir, bx.tcx()); 
        
               let discr_ty = self.monomorphize(discr_ty); 
        
               let discr = self 
        
                   .codegen_place(&mut bx, place.as_ref()) 
        
                   .codegen_get_discr(&mut bx, discr_ty); 
        
               ( 
        
                   bx, 
        
                   OperandRef { 
        
                       val: OperandValue::Immediate(discr), 
        
                       layout: self.cx.layout_of(discr_ty), 
        
                   }, 
        
               ) 
        
           }

This suggests another avenue for these casts: enum->integer casts should be lowered in MIR to Rvalue::Discriminant( (which would produce an integer of the discriminant type) followed by the original cast to integer. It also has the advantage that it would work on enums with data just as well, if we ever want to support those in as casts.

eddyb · 2022-05-07T17:34:06Z

cc @nagisa wrt the above comment, and because of the codegen logic that #75529 touched - if we go the route of not having direct enum->integer casts in MIR (but instead relying on Rvalue::Discriminant, we would have no obvious place to put that assume.

However, it strikes me that today enum->integer casts can be by-value in codegen, whereas Rvalue::Discriminant is memory-only (i.e. requires a PlaceRef) for now. So instead of the assume we'd get a load with !range metadata attached, that might be just as good?

(And/or if/when we get the ability to read discriminants from OperandRefs directly, that could be where the assume would go, replacing the !range metadata?)

RalfJung · 2022-05-07T17:42:53Z

The llval value just before the actual int2int cast would be identical to this I'm guessing:

I tried that, but couldn't complete the type golf since I didn't have a place to work with.

This suggests another avenue for these casts: enum->integer casts should be lowered in MIR to Rvalue::Discriminant( (which would produce an integer of the discriminant type) followed by the original cast to integer. It also has the advantage that it would work on enums with data just as well, if we ever want to support those in as casts.

Yeah that would be nice, make both Miri's and codegen's job much easier. :D
But MIR building is even scarier than codegen, so I doubt I can do that...

nagisa · 2022-05-07T20:38:36Z

The enum as scalar cast was always intended to behave as if it was reading out a discriminant, wasn’t it? I'm quite surprised this isn't already happening.

I'm perfectly content with the idea of us losing assumes that have been added before if it helps some agenda that's grander than micro-optimizing a handcrafted test case. I said as such in the linked PR:

I don’t mind adding this either, but I wouldn’t hold my breath that any specific behaviour will last, especially across backend versions.

!range definitely sounds much more palatable and resilient approach too.

RalfJung · 2022-05-08T07:19:54Z

The enum as scalar cast was always intended to behave as if it was reading out a discriminant, wasn’t it? I'm quite surprised this isn't already happening.

Good, so looks like we have broad agreement here. Then we seem to have 2 options:

Either this happens already during MIR building. Then we need someone familiar with that code to pick up this PR; that's a bit too much foreign code for me to pick up right now.
Or the codegen backends all do something like that themselves. I did that for Miri; with a little help hopefully I can get it done for the SSA backend. The main problem is that I have an OperandRef but to call the get disicriminant code I need a PlaceRef, and I don't know how to convert from one to the other. In Miri we have force_allocation. In codegen, does something like that exist -- is that even possible? Or do we need to adjust the SSA logic to not treat "enums that are inputs to cast" as by-value so that we are sure we have a by-ref here and somehow can get that by-ref information out of the OperandRef?

Doing it in the MIR seems more elegant, so if we can get some MIR people to help here that would be great. :)

oli-obk · 2022-05-08T12:38:51Z

Then we need someone familiar with that code to pick up this PR; that's a bit too much foreign code for me to pick up right now.

I'll create a PR

compiler/rustc_const_eval/src/interpret/cast.rs

oli-obk · 2022-05-09T11:48:50Z

I'll create a PR

done: #96862

I'm perfectly content with the idea of us losing assumes that have been added before if it helps some agenda that's grander than micro-optimizing a handcrafted test case.

We lost some of them. I tried to get them back by telling LLVM more things about the result of the discriminant (the input automatically gets that via Operand::load). That ended up giving lots of miscompilations. I think the right level of changing things would be in LLVM opts, as it works for enum Foo { A, B }, but just about nothing else (like not even enum Foo { A = 1, B = 2 } or various other repesentation changes).

bors · 2022-05-10T10:54:11Z

☔ The latest upstream changes (presumably #96891) made this pull request unmergeable. Please resolve the merge conflicts.

jackh726 · 2022-05-22T16:41:17Z

r? @eddyb

Change enum->int casts to not go through MIR casts. follow-up to rust-lang#96814 this simplifies all backends and even gives LLVM more information about the return value of `Rvalue::Discriminant`, enabling optimizations in more cases.

rustbot · 2022-07-05T15:21:22Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

compiler/rustc_const_eval/src/interpret/cast.rs

RalfJung · 2022-07-05T15:29:21Z

Now that #96862 landed, this becomes easy. :) (Other than convincing github to not show outdated diffs.)
@oli-obk ready for review again.

oli-obk · 2022-07-05T15:56:36Z

src/test/ui/aligned_enum_cast.rs

@@ -11,5 +11,13 @@ enum Aligned {
 fn main() {
    let aligned = Aligned::Zero;
    let fo = aligned as u8;
-    println!("foo {}",fo);
+    println!("foo {}", fo);
+    println!("{}", tou8(Aligned::Zero));


should this be an assertion instead of a print? Otherwise we're only testing that this code runs, not that it does the right thing

I have added assertions but kept the prints, just in case they were relevant to trigger the ICE.

oli-obk

r=me with an explanation for the prints or changed to an assert

RalfJung · 2022-07-05T17:31:08Z

@bors r=oli-obk

bors · 2022-07-05T17:31:10Z

📌 Commit d5721ce has been approved by oli-obk

…laumeGomez Rollup of 6 pull requests Successful merges: - rust-lang#95503 (bootstrap: Allow building individual crates) - rust-lang#96814 (Fix repr(align) enum handling) - rust-lang#98256 (Fix whitespace handling after where clause) - rust-lang#98880 (Proper macOS libLLVM symlink when cross compiling) - rust-lang#98944 (Edit `rustc_mir_dataflow::framework::lattice::FlatSet` docs) - rust-lang#98951 (Update books) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

assert Scalar sanity With rust-lang#96814 having landed, finally our `Scalar` layouts have the invariants they deserve. :)

Change enum->int casts to not go through MIR casts. follow-up to rust-lang/rust#96814 this simplifies all backends and even gives LLVM more information about the return value of `Rvalue::Discriminant`, enabling optimizations in more cases.

rust-highfive assigned jackh726 May 7, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 7, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 7, 2022

This comment has been minimized.

# to view

eddyb suggested changes May 7, 2022

View reviewed changes

compiler/rustc_middle/src/ty/layout.rs Outdated Show resolved Hide resolved

compiler/rustc_const_eval/src/interpret/cast.rs Outdated Show resolved Hide resolved

This comment has been minimized.

# to view

bjorn3 reviewed May 8, 2022

View reviewed changes

compiler/rustc_const_eval/src/interpret/cast.rs Outdated Show resolved Hide resolved

oli-obk mentioned this pull request May 9, 2022

Change enum->int casts to not go through MIR casts. #96862

Merged

rust-highfive assigned eddyb and unassigned jackh726 May 22, 2022

RalfJung added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 27, 2022

RalfJung force-pushed the enum-repr-align branch from 7d95b1e to a57e43b Compare July 5, 2022 15:21

RalfJung commented Jul 5, 2022

View reviewed changes

compiler/rustc_const_eval/src/interpret/cast.rs Outdated Show resolved Hide resolved

RalfJung force-pushed the enum-repr-align branch from a57e43b to 58cf734 Compare July 5, 2022 15:28

oli-obk reviewed Jul 5, 2022

View reviewed changes

oli-obk approved these changes Jul 5, 2022

View reviewed changes

This comment has been minimized.

# to view

fix the layout of repr(align) enums

cedc428

RalfJung force-pushed the enum-repr-align branch from 58cf734 to cedc428 Compare July 5, 2022 17:24

add asserts

d5721ce

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jul 5, 2022

GuillaumeGomez mentioned this pull request Jul 5, 2022

Rollup of 6 pull requests #98963

Merged

bors merged commit 3e802d7 into rust-lang:master Jul 6, 2022

rustbot added this to the 1.64.0 milestone Jul 6, 2022

RalfJung mentioned this pull request Jul 6, 2022

assert Scalar sanity #98968

Merged

RalfJung deleted the enum-repr-align branch July 6, 2022 02:27

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 6, 2022

Rollup merge of rust-lang#98968 - RalfJung:scalar-sanity, r=oli-obk

7f62a71

assert Scalar sanity With rust-lang#96814 having landed, finally our `Scalar` layouts have the invariants they deserve. :)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix repr(align) enum handling #96814

Fix repr(align) enum handling #96814

RalfJung commented May 7, 2022

rust-highfive commented May 7, 2022

rust-highfive commented May 7, 2022

This comment has been minimized.

eddyb left a comment

eddyb commented May 7, 2022

eddyb commented May 7, 2022

RalfJung commented May 7, 2022

nagisa commented May 7, 2022

RalfJung commented May 8, 2022

This comment has been minimized.

oli-obk commented May 8, 2022

oli-obk commented May 9, 2022

bors commented May 10, 2022

jackh726 commented May 22, 2022

rustbot commented Jul 5, 2022

RalfJung commented Jul 5, 2022

oli-obk Jul 5, 2022

RalfJung Jul 5, 2022

oli-obk left a comment

This comment has been minimized.

RalfJung commented Jul 5, 2022

bors commented Jul 5, 2022

Fix repr(align) enum handling #96814

Fix repr(align) enum handling #96814

Conversation

RalfJung commented May 7, 2022

rust-highfive commented May 7, 2022

rust-highfive commented May 7, 2022

This comment has been minimized.

eddyb left a comment

Choose a reason for hiding this comment

eddyb commented May 7, 2022

eddyb commented May 7, 2022

RalfJung commented May 7, 2022

nagisa commented May 7, 2022

RalfJung commented May 8, 2022

This comment has been minimized.

oli-obk commented May 8, 2022

oli-obk commented May 9, 2022

bors commented May 10, 2022

jackh726 commented May 22, 2022

rustbot commented Jul 5, 2022

RalfJung commented Jul 5, 2022

oli-obk Jul 5, 2022

Choose a reason for hiding this comment

RalfJung Jul 5, 2022

Choose a reason for hiding this comment

oli-obk left a comment

Choose a reason for hiding this comment

This comment has been minimized.

RalfJung commented Jul 5, 2022

bors commented Jul 5, 2022