Stream: t-compiler/wg-mir-opt

Topic: Is changing tuple reprs in const_prop OK?


osa1 (Dec 04 2019 at 13:34, on Zulip):

I'm looking at a few const_prop bugs (#66971 and #67019) -- should const_prop be changing representations of tuple values at all? I think the problem may be more general than just values represented as two values (ScalarPairs). Here's an example program:

fn test(this: ((u8,),)) {
    assert!((this.0).0 == 0);
}

fn main() {
    test(((0,),));
}

const_prop changes the argument tuple from

        _3 = (const 0u8,);
        _2 = (move _3,);

to

        _3 = const Scalar(0x00) : (u8,);
        _2 = const Scalar(0x00) : ((u8,),);

While this is probably correct in the sense that the ((0,),) is represented the same as 0 in the final assembly, I'm not sure if this is something that const_prop is supposed to be doing. Is this expected? Can anyone comment on this?

Wesley Wiser (Dec 04 2019 at 13:49, on Zulip):

I think I agree with you. MIR optimizations should try to avoid depending on the layout of types as much as possible IMO.

Wesley Wiser (Dec 04 2019 at 13:49, on Zulip):

@oli Do you have thoughts? ^

oli (Dec 04 2019 at 15:09, on Zulip):

what do you mean by "depending on the layout of types"?

oli (Dec 04 2019 at 15:09, on Zulip):

I think using the layout is totally something we should do (same with the optimization shown above where we get trivial constants for tuples and such)

Wesley Wiser (Dec 04 2019 at 15:16, on Zulip):

Hmm... I guess my feeling was that if you have an n-element tuple, you should always have n operands when assigning to that tuple even if some of the inner types are ZSTs.

Wesley Wiser (Dec 04 2019 at 15:16, on Zulip):

However, I think the most important thing is consistency with the rest of the MIR pipeline. If everyone is already using layout to do various computations, we should do that as well.

oli (Dec 04 2019 at 15:17, on Zulip):

oh yea, definitely, iirc @osa1 already fixed that in their PR

Wesley Wiser (Dec 04 2019 at 15:17, on Zulip):

I don't want other passes to have care whether or not ConstProp has run. So the MIR should be just as consistent after ConstProp runs as it is before

oli (Dec 04 2019 at 15:17, on Zulip):

yes, definitely, the previous code was just broken

Wesley Wiser (Dec 04 2019 at 15:18, on Zulip):

Agreed

oli (Dec 04 2019 at 15:18, on Zulip):

cool, all on one boat, I was just misreading

Wesley Wiser (Dec 04 2019 at 15:18, on Zulip):

No, it's probably my bad. I haven't had :coffee: yet

osa1 (Dec 05 2019 at 07:05, on Zulip):

Hmm, so it's fine for const-prop to turns a (0,) to 0? I was thinking that that's accidentally done as it changes the logical representation of the variable.

oli (Dec 05 2019 at 12:17, on Zulip):

Huh, that shouldn't happen, but a Scalar(0) does not mean a 0, it can still be a (0,).

oli (Dec 05 2019 at 12:17, on Zulip):

In memory a single elem tuple looks the same as the single elem. The same is true for constants in rustc

osa1 (Dec 05 2019 at 12:28, on Zulip):

That does not happen, sorry my example is not quite right (though the question is valid) -- I should've said "turns a ((0, 1),) to a (0, 1)". Agreed that they're represented the same in runtime.

oli (Dec 05 2019 at 12:41, on Zulip):

Yea, that's wrong

osa1 (Dec 05 2019 at 12:49, on Zulip):

Sorry, your answers are a bit too concise for me to understand. Maybe you can respond to my question here https://github.com/rust-lang/rust/issues/67019 ?

oli (Dec 05 2019 at 12:56, on Zulip):

Sorry about that. I'll write up sth more detailed when I get to a PC

osa1 (Dec 05 2019 at 14:38, on Zulip):

Thanks

oli (Dec 06 2019 at 10:16, on Zulip):

I made a writeup on the issue

oli (Dec 06 2019 at 10:18, on Zulip):

If you want to just fix the ICE for now, you can also just bail out if the tuple doesn't have exactly two fields with Scalar layout

oli (Dec 06 2019 at 10:19, on Zulip):

this allows the full fix (actually more features, because we'd const prop more) to bake in a separate PR and makes your PR easier to backport in case we need to move it to beta

osa1 (Dec 06 2019 at 11:01, on Zulip):

Thanks @oli, that's really helpful. I think I understand the basic idea, I'll give it a try.

osa1 (Dec 06 2019 at 11:55, on Zulip):

@oli I'm trying to implement the idea. So far I added two lines to replace_with_const:

        let _x = crate::const_eval::op_to_const(&crate::const_eval::mk_eval_cx(self.tcx, DUMMY_SP, self.param_env), value);
        debug!("op_to_const = {:#?}", _x);

but that triggered an assertion in const_eval:

thread 'rustc' panicked at 'assertion failed: `(left == right)`
  left: `8`,
 right: `1`', src/librustc/mir/interpret/value.rs:319:9
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /home/omer/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/libunwind.rs:88
   1: backtrace::backtrace::trace_unsynchronized
             at /home/omer/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print_fmt
             at src/libstd/sys_common/backtrace.rs:84
   3: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
             at src/libstd/sys_common/backtrace.rs:61
   4: core::fmt::write
             at src/libcore/fmt/mod.rs:1025
   5: std::io::Write::write_fmt
             at src/libstd/io/mod.rs:1428
   6: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:65
   7: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:50
   8: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:193
   9: std::panicking::default_hook
             at src/libstd/panicking.rs:210
  10: <alloc::boxed::Box<F> as core::ops::function::Fn<A>>::call
             at ./src/liballoc/boxed.rs:983
  11: rustc_driver::report_ice
             at src/librustc_driver/lib.rs:1174
  12: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:475
  13: rust_begin_unwind
             at src/libstd/panicking.rs:375
  14: std::panicking::begin_panic_fmt
             at src/libstd/panicking.rs:326
  15: rustc::mir::interpret::value::Scalar<Tag>::check_raw
             at ./<::std::macros::panic macros>:9
  16: rustc::mir::interpret::value::Scalar<Tag>::to_bits
             at ./src/librustc/mir/interpret/value.rs:329
  17: rustc::mir::interpret::value::Scalar<Tag>::to_machine_usize
             at ./src/librustc/mir/interpret/value.rs:409
  18: rustc::mir::interpret::value::ScalarMaybeUndef<Tag>::to_machine_usize
             at ./src/librustc/mir/interpret/value.rs:569
  19: rustc_mir::const_eval::op_to_const
             at src/librustc_mir/const_eval.rs:121
  20: rustc_mir::transform::const_prop::ConstPropagator::replace_with_const
             at src/librustc_mir/transform/const_prop.rs:624
...

Am I misusing op_to_const or is this something to debug and fix?

oli (Dec 06 2019 at 12:01, on Zulip):

you should not create a new eval_cx, but reuse the one from the const propagator. You'll have to make op_to_const generic over the machine parameter in order to do this

oli (Dec 06 2019 at 12:02, on Zulip):

I can't tell if this is the cause of the ICE though, but without that change you'd run into errors later because some allocations don't exist outside the const prop machine

osa1 (Dec 06 2019 at 12:04, on Zulip):

OK, I'll try that. 1 == 8 seems like the LHS is in words but RHS is in bytes so maybe there's a confusion there, but I'm not sure. I'll check.

Wesley Wiser (Dec 06 2019 at 12:04, on Zulip):

you should not create a new eval_cx, but reuse the one from the const propagator. You'll have to make op_to_const generic over the machine parameter in order to do this

There's a typedef I introduced the last time I had to do that which you can probably reuse. https://doc.rust-lang.org/nightly/nightly-rustc/rustc_mir/const_eval/type.CompileTimeEvalContext.html

oli (Dec 06 2019 at 12:05, on Zulip):

Yea the assert failure is a Scalar that is one byte being accessed with a type that is 8. The 8 is correct, since the code uses to_machine_usize, which on a 64bit system looks for an 8 byte value

oli (Dec 06 2019 at 12:07, on Zulip):

Ok, I found the ICE

oli (Dec 06 2019 at 12:07, on Zulip):

lol what on earth was I thinking?

oli (Dec 06 2019 at 12:07, on Zulip):

I think this may be triggerable on stable

oli (Dec 06 2019 at 12:08, on Zulip):

https://github.com/rust-lang/rust/blob/7b482cdf7ce55e05ee8392e1ade70966e3189cfd/src/librustc_mir/const_eval.rs#L108 does not check for slice type and just assumes scalar pairs are slices

osa1 (Dec 06 2019 at 12:08, on Zulip):

:sweat_smile:

oli (Dec 06 2019 at 12:10, on Zulip):

ah no. we have checks: https://github.com/rust-lang/rust/blob/7b482cdf7ce55e05ee8392e1ade70966e3189cfd/src/librustc_mir/const_eval.rs#L67

oli (Dec 06 2019 at 12:10, on Zulip):

phew

oli (Dec 06 2019 at 12:11, on Zulip):

But the ICE is still there because https://github.com/rust-lang/rust/blob/7b482cdf7ce55e05ee8392e1ade70966e3189cfd/src/librustc_mir/const_eval.rs#L80 is wrong in the const prop usage

osa1 (Dec 06 2019 at 12:13, on Zulip):

Hmm, right

oli (Dec 06 2019 at 12:14, on Zulip):

Ok... this is not too fun. I guess you need to split up op_to_const into three parts.

  1. https://github.com/rust-lang/rust/blob/7b482cdf7ce55e05ee8392e1ade70966e3189cfd/src/librustc_mir/const_eval.rs#L58-L76
  2. https://github.com/rust-lang/rust/blob/7b482cdf7ce55e05ee8392e1ade70966e3189cfd/src/librustc_mir/const_eval.rs#L77-L86
  3. https://github.com/rust-lang/rust/blob/7b482cdf7ce55e05ee8392e1ade70966e3189cfd/src/librustc_mir/const_eval.rs#L77-L131
oli (Dec 06 2019 at 12:15, on Zulip):

The first and third part is what we can reuse, but the second part we need to change the else-part to actually force allocate the value we got

osa1 (Dec 06 2019 at 12:15, on Zulip):

So I'm still trying to find this eval_cx in const-prop. I don't think const-prop has an eval_cx, does it? I can't find it

oli (Dec 06 2019 at 12:15, on Zulip):

At this point I really suggest you make a small PR just fixing the bug as described earlier, and make the full change in a separate PR, because otherwise we won't be able to tell apart problems from the small fix from problems in the big fix

osa1 (Dec 06 2019 at 12:16, on Zulip):

Oh, I think I found it.

oli (Dec 06 2019 at 12:16, on Zulip):

XD you get it by calling use_ecx :D

oli (Dec 06 2019 at 12:16, on Zulip):

you can't use it directly

oli (Dec 06 2019 at 12:16, on Zulip):

(or you shouldn't)

osa1 (Dec 06 2019 at 12:18, on Zulip):

Hmm, InterpCx<'mir, 'tcx, ConstPropMachine> is what I have in const-prop, but I really need InterpCx<'mir, 'tcx, CompileTimeInterpreter<'mir, 'tcx>>. Can I use the former for the latter? The Machine arguments are different

osa1 (Dec 06 2019 at 12:20, on Zulip):

At this point I really suggest you make a small PR just fixing the bug as described earlier, and make the full change in a separate PR, because otherwise we won't be able to tell apart problems from the small fix from problems in the big fix

The bug described earlier is #66971 ? Or something else?

oli (Dec 06 2019 at 12:20, on Zulip):

No, you can't. You need to make op_to_const generic over the machine

oli (Dec 06 2019 at 12:20, on Zulip):

If you want to just fix the ICE for now, you can also just bail out if the tuple doesn't have exactly two fields with Scalar layout

this is the "fix explanation", I can make it more detailed

osa1 (Dec 06 2019 at 12:21, on Zulip):

this is the "fix explanation", I can make it more detailed

OK, that makes sense though that disables the optimization when type and ABI layout doesn't match... which may be fine I guess.

osa1 (Dec 06 2019 at 12:22, on Zulip):

I'll do that and then we can discuss the next steps if that's OK.

oli (Dec 06 2019 at 12:22, on Zulip):

well, the bug being fixed by checking that it's a two element tuple with each field being Scalar layout is both #66971 and #67019

oli (Dec 06 2019 at 12:22, on Zulip):

yea, let's pessimize for now, there's no nice way to resolve this without adding more (and possibly more fragile) code

osa1 (Dec 06 2019 at 12:24, on Zulip):

Alright. Just curious about the plan, is this optimization (when type and ABI layout doesn't match) something we want to do in the future or are we shelving it?

oli (Dec 06 2019 at 12:27, on Zulip):

we want do do it. The type and ABI layout do match, it's just that they are entirely orthogonal ways to think about a type. It is no conflict that the layout of a three element tuple is a ScalarPair if one of the elements is a ZST

oli (Dec 06 2019 at 12:28, on Zulip):

The bug occurred because we forgot that these things are independent ways to look at values and should not be mixed

osa1 (Dec 06 2019 at 13:23, on Zulip):

@oli

bail out if the tuple doesn't have exactly two fields with Scalar layout

How do I check if the type has two fields with Scalar layout?

oli (Dec 06 2019 at 13:30, on Zulip):

you check if let Abi::Scalar(_) = ecx.layout_of(field_ty)?.details.abi (inside a use_ecx). You get the field types from the if let ty::Tuple(substs) = ty {

osa1 (Dec 06 2019 at 14:11, on Zulip):

OK, I have a patch. Starting testing now.

osa1 (Dec 06 2019 at 14:31, on Zulip):

Sigh I accidentally cleaned the tree so I'll have to build rustc from scratch now. I'll update the PR in an hour or so.

oli (Dec 06 2019 at 14:31, on Zulip):

XD oh no

osa1 (Dec 06 2019 at 14:31, on Zulip):

But the good news is the test passes. I'll try the other relevant issues now once I have a working build again

osa1 (Dec 06 2019 at 14:32, on Zulip):

After this I'd be happy to work on the more general solution to propagating pairs

Wesley Wiser (Dec 06 2019 at 14:33, on Zulip):

Awesome!

Wesley Wiser (Dec 06 2019 at 14:34, on Zulip):

I think your changes will fix a crash I'm seeing locally when I try to allow propagation into user variables.

osa1 (Dec 06 2019 at 14:48, on Zulip):

PR updated. It fixes 3 issues that I know of.

oli (Dec 06 2019 at 14:49, on Zulip):

:party_ball:

Last update: Apr 05 2020 at 00:15UTC