Stream: t-compiler/wg-mir-opt

Topic: SimplifyArmIdentity on matching layouts

Simon Vandel Sillesen (Jul 26 2020 at 11:26, on Zulip):

I am trying to resolve https://github.com/rust-lang/rust/blob/13f9aa190957b993a268fd4a046fce76ca8814ee/src/librustc_mir/transform/simplify_try.rs#L292 such that the optmization can run on code like this:

enum MyResult<T,E>{
    Ok(T),
    Err(E)
}

fn id(r: Result<u8, i32>) -> MyResult<u8, i32> {
    match r {
        Ok(x) => MyResult::Ok(x),
        Err(y) => MyResult::Err(y),
    }
}

My current progress is that I have changed the condition so that it checks if the layouts match. My problem arrives when trying to optimize the above test case.

encountered `Assign` statement with incompatible types:
left-hand side has type: MyResult<u8, i32>
right-hand side has type: std::result::Result<u8, i32>
  --> /home/simon/Documents/rust/src/test/mir-opt/simplify-arm-same-layout.rs:11:18
   |
11 |         Ok(x) => MyResult::Ok(x),
   |                  ^^^^^^^^^^^^^^^
   |
   = note: delayed at src/librustc_mir/transform/validate.rs:140:36

The layouts match, but the types don't. How would I align the types so the validater won't complain? I see a Cast RValue, but is that only for primitives/variant casting? Else, a call to transmute could work. But then the inliner has to then inline that call, which seems like a roundabout way of doing it.
Any other tricks?

oli (Jul 26 2020 at 11:35, on Zulip):

hmm... I've wanted type-ignorant assignments before... Maybe we should just have a flag on StatementKind::Assign that says whether the assignment is actually a transmute (we could even lower transmute calls to such assignments then!) cc @RalfJ @WG-mir-opt

RalfJ (Jul 27 2020 at 12:32, on Zulip):

I see this as a design decision we could have either way: do we want assignment to well-typed in MIR or not?

RalfJ (Jul 27 2020 at 12:33, on Zulip):

these optimizations provide a motivation for at least also having an untyped ("transmuting") assignment operation, though I cannot judge if the optimization will be useful enough to be worth that trouble. if yes, we could either say that all assignments are "transmuting" or we add a flag.

RalfJ (Jul 27 2020 at 12:34, on Zulip):

I am not entirely sure what benefit we would have from that flag though.

oli (Jul 27 2020 at 12:43, on Zulip):

RalfJ said:

I am not entirely sure what benefit we would have from that flag though.

mainly MIR simplicity, as we get rid of transmute calls in terminators, but since we don't have first class support for other intrinsics... Not sure how much sense that makes. I think at the rust all hands @eddyb and others talked about adding non-unwinding function calls as statements in MIR, maybe that would be a better approach for this, too

nagisa (Jul 27 2020 at 14:22, on Zulip):

extended basic blocks.

nagisa (Jul 27 2020 at 14:22, on Zulip):

https://github.com/rust-lang/rust/issues/39685

oli (Jul 27 2020 at 14:38, on Zulip):

thanks! Right, that was even more powerful

RalfJ (Jul 29 2020 at 16:30, on Zulip):

@oli

mainly MIR simplicity, as we get rid of transmute calls in terminators

I meant, what does the flag gain us compared to just allowing transmute on all assignments

oli (Jul 29 2020 at 16:31, on Zulip):

oh... no accidental introduction of transmuting assignments?

RalfJ (Jul 29 2020 at 16:32, on Zulip):

hm, maybe it helps as a safety net... not sure it's worth the effort though. but then, we already did all the work of implementing the check.^^

oli (Jul 29 2020 at 16:34, on Zulip):

oh suddenly you think safety nets may not be worth the effort? Scalar::Bits { size, ..} cough cough :laughing:

oli (Jul 29 2020 at 16:35, on Zulip):

joking aside, Until we have a good argument to remove it, I think we should just keep it if it doesn't have any perf or memory regressions

oli (Jul 29 2020 at 16:35, on Zulip):

of course we need to be careful around StatementKind and increasing its size

bjorn3 (Jul 29 2020 at 16:35, on Zulip):

The no transmuting assignment assertion in cg_clif has saved me countless times.

RalfJ (Jul 31 2020 at 08:27, on Zulip):

oli said:

oh suddenly you think safety nets may not be worth the effort? Scalar::Bits { size, ..} cough cough :laughing:

fair, but that safety nat has proven to be effective at catching bugs. ;) the other one -- so far not, did it?

RalfJ (Jul 31 2020 at 08:28, on Zulip):

but it looks like it was useful for @bjorn3 so that's a good argument

RalfJ (Jul 31 2020 at 08:29, on Zulip):

@bjorn3 do you have your own check there and what does it look like? it turned out to be surprisingly non-trivial to properly check for this, see https://github.com/rust-lang/rust/blob/ac91673d895a0c578ed773e1280bdde8adb87b8c/src/librustc_mir/transform/validate.rs#L183

bjorn3 (Jul 31 2020 at 09:13, on Zulip):

This is the check I use: https://github.com/bjorn3/rustc_codegen_cranelift/blob/c5899526e4a6244ca200900e3f825c72e8ad5559/src/value_and_place.rs#L411-L462

Basically it matches on the type. If it is a kind that has late bound regions then those are erased and it recurses. Otherwise it asserts that the types are equal. All early bound lifetimes are already erased by fx.layout_of before that function is reached.

RalfJ (Aug 05 2020 at 15:58, on Zulip):

@bjorn3 okay so this should be functionally equivalent to https://github.com/rust-lang/rust/blob/ac91673d895a0c578ed773e1280bdde8adb87b8c/src/librustc_mir/transform/validate.rs#L41 ?

bjorn3 (Aug 05 2020 at 16:30, on Zulip):

@RalfJ Yes, except that it also ignores mutability of references. At least in the past casting from &mut T to &T could happen during assignments. I don't know if this is still true though.

Last update: Jan 22 2021 at 13:00UTC