Stream: t-compiler/wg-mir-opt

Topic: Support indirects on const-prop


Christian Poveda (May 31 2019 at 20:54, on Zulip):

great

Christian Poveda (May 31 2019 at 20:54, on Zulip):

ohh and I need a type for that constant

Christian Poveda (May 31 2019 at 20:55, on Zulip):

should I take it from Mplace.layout.ty?

Wesley Wiser (May 31 2019 at 20:55, on Zulip):

Yeah, I believe that's already the ty you need

Christian Poveda (May 31 2019 at 20:59, on Zulip):

ok i believe I'm almost there

Christian Poveda (May 31 2019 at 20:59, on Zulip):

ok, to generate an OpTy I used eval_operand, but it needs a SourceInfo

Wesley Wiser (May 31 2019 at 21:05, on Zulip):

replace_with_const already takes a Span, so I'd just add it as another parameter

Christian Poveda (May 31 2019 at 23:58, on Zulip):

But then I'll have to propagate that SourceInfo on every replace_with_const call

Wesley Wiser (Jun 01 2019 at 00:10, on Zulip):

I think you just need to change replace-with_const's Span argument to be a SourceLocation

Wesley Wiser (Jun 01 2019 at 00:10, on Zulip):

Then here at at the call site https://github.com/rust-lang/rust/blob/75f464481ed8c924086fc0b9a2d31841bbdbcabd/src/librustc_mir/transform/const_prop.rs#L658

Wesley Wiser (Jun 01 2019 at 00:10, on Zulip):

Just pass statement.source_info

Christian Poveda (Jun 01 2019 at 01:43, on Zulip):

ok will try

Christian Poveda (Jun 01 2019 at 15:12, on Zulip):

I'm done with the changes :D, should I pr?

Wesley Wiser (Jun 01 2019 at 16:59, on Zulip):

Yeah, definitely!!

Christian Poveda (Jun 01 2019 at 17:37, on Zulip):

a test failed D:

Christian Poveda (Jun 01 2019 at 17:37, on Zulip):

run-pass/mir/mir-inlining/no-trait-method-issue-40473.rs

Christian Poveda (Jun 01 2019 at 17:37, on Zulip):

i dont think that's my fault

Wesley Wiser (Jun 01 2019 at 20:10, on Zulip):

Hmmm looks like it failed again

Wesley Wiser (Jun 01 2019 at 20:10, on Zulip):

I'm building locally...

Christian Poveda (Jun 01 2019 at 20:36, on Zulip):

I don't understand why is this affecting trait solving

Wesley Wiser (Jun 01 2019 at 20:43, on Zulip):

/me waiting for llvm to finish building...

Wesley Wiser (Jun 02 2019 at 00:04, on Zulip):

Ok, I'm guessing this has something to do with the failing ptr being a null ptr.

Wesley Wiser (Jun 02 2019 at 00:12, on Zulip):

Nope, still failing...

Christian Poveda (Jun 02 2019 at 00:45, on Zulip):

which failing ptr?

Wesley Wiser (Jun 02 2019 at 01:07, on Zulip):
TRACE 2019-06-02T00:45:18Z: rustc_mir::transform::const_prop: attepting to replace const <&usize as std::fmt::Debug>::fmt as for<'r, 's, 't0> fn(&'r &usize, &'s mut std::fmt::Formatter<'t0>) -> std::result::Result<(), std::fmt::Error> (Pointer(ReifyFnPointer)) with OpTy { op: Indirect(MemPlace { ptr: AllocId(0).0x0, align: Align { pow2: 3 }, meta: None }), layout: TyLayout { ty: for<'r, 's, 't0> fn(&'r &usize, &'s mut std::fmt::Formatter<'t0>) -> std::result::Result<(), std::fmt::Error>, details: LayoutDetails { variants: Single { index: 0 }, fields: Union(0), abi: Scalar(Scalar { value: Pointer, valid_range: 1..=18446744073709551615 }), align: AbiAndPrefAlign { abi: Align { pow2: 3 }, pref: Align { pow2: 3 } }, size: Size { raw: 8 } } } }
TRACE 2019-06-02T00:45:18Z: rustc_mir::transform::const_prop: tag: ()
TRACE 2019-06-02T00:45:18Z: rustc_mir::transform::const_prop: Indirect: AllocId(0).0x0
TRACE 2019-06-02T00:45:18Z: rustc_mir::transform::const_prop: op: const Scalar(AllocId(0).0x0) : for<'r, 's, 't0> fn(&'r &usize, &'s mut std::fmt::Formatter<'t0>) -> std::result::Result<(), std::fmt::Error>
TRACE 2019-06-02T00:45:18Z: rustc_mir::transform::const_prop: opty: OpTy { op: Immediate(Scalar(AllocId(0).0x0)), layout: TyLayout { ty: for<'r, 's, 't0> fn(&'r &usize, &'s mut std::fmt::Formatter<'t0>) -> std::result::Result<(), std::fmt::Error>, details: LayoutDetails { variants: Single { index: 0 }, fields: Union(0), abi: Scalar(Scalar { value: Pointer, valid_range: 1..=18446744073709551615 }), align: AbiAndPrefAlign { abi: Align { pow2: 3 }, pref: Align { pow2: 3 } }, size: Size { raw: 8 } } } }
thread 'rustc' panicked at 'called `Option::unwrap()` on a `None` value', src/libcore/option.rs:347:21
Christian Poveda (Jun 02 2019 at 01:36, on Zulip):

where does this unwrap comes from? I though that const-prop just stops when it cannot propagate

Wesley Wiser (Jun 02 2019 at 01:41, on Zulip):

Yeah, I'm not sure. I can't seem to get a backtrace on my mac

Wesley Wiser (Jun 02 2019 at 02:22, on Zulip):

Ok, it looks like what happened is that your changes allowed more const evaluation to happen elsewhere and that's what's causing the ICE

Wesley Wiser (Jun 02 2019 at 02:22, on Zulip):

Specifically, the test ICEs because it tries to const eval this:

_42 = const <&usize as std::fmt::Debug>::fmt as for<'r, 's, 't0> fn(&'r &usize, &'s mut std::fmt::Formatter<'t0>) -> std::result::Result<(), std::fmt::Error> (Pointer(ReifyFnPointer))

and that doesn't seem to work at all

Wesley Wiser (Jun 02 2019 at 02:24, on Zulip):

So probably somewhere around here https://github.com/rust-lang/rust/blob/46e0f2721a8f8fe84588e081c96ace5aecf25a32/src/librustc_mir/transform/const_prop.rs#L372

Wesley Wiser (Jun 02 2019 at 02:25, on Zulip):

We need to check if kind is a CastKind::Pointer(_) and return None in that case.

Wesley Wiser (Jun 02 2019 at 02:49, on Zulip):

@oli might have a better suggestion

Christian Poveda (Jun 02 2019 at 04:44, on Zulip):

I did the changes and I'm testing now

Christian Poveda (Jun 02 2019 at 07:10, on Zulip):

It failed again, maybe this is propagating elsewhere?

Wesley Wiser (Jun 02 2019 at 16:49, on Zulip):

The ui tests are all passing now so that's good!

Wesley Wiser (Jun 02 2019 at 16:49, on Zulip):

There's 4 failing MIR optimization tests. I think they're all ICEs.

Wesley Wiser (Jun 02 2019 at 16:49, on Zulip):

I've got a fix for one and I'm investigating the others

Wesley Wiser (Jun 02 2019 at 21:18, on Zulip):

@Christian Poveda I pushed some fixes into your branch per @oli's comments on GitHub

Christian Poveda (Jun 03 2019 at 00:25, on Zulip):

Thank you!

Wesley Wiser (Jun 03 2019 at 11:49, on Zulip):

@oli you around?

oli (Jun 03 2019 at 11:49, on Zulip):

jop

Wesley Wiser (Jun 03 2019 at 11:49, on Zulip):

Got a minute to chat about this pr?

oli (Jun 03 2019 at 11:49, on Zulip):

yes

Wesley Wiser (Jun 03 2019 at 11:50, on Zulip):

So your comment here is basically how I understand this to work.

Wesley Wiser (Jun 03 2019 at 11:50, on Zulip):

So I guess I'm either not understanding something (probably that) or I'm confused why you don't think this should work.

Wesley Wiser (Jun 03 2019 at 11:51, on Zulip):

Do steps 1-5 make sense to you?

oli (Jun 03 2019 at 11:51, on Zulip):

4 and 5 don't make sense to me considering 1-3 happened

Wesley Wiser (Jun 03 2019 at 11:51, on Zulip):

ok

oli (Jun 03 2019 at 11:52, on Zulip):

so basically what I thought was the situation is that Operand::Immediate refers to the memory, e.g. where the 5 in 5 as u8 is stored

oli (Jun 03 2019 at 11:52, on Zulip):

so step 2 gives you an Immediate::Scalar(5)

oli (Jun 03 2019 at 11:53, on Zulip):

and then we turn that 5 into a pointer?

oli (Jun 03 2019 at 11:53, on Zulip):

and create a ByRef whose location is 5?

Wesley Wiser (Jun 03 2019 at 11:53, on Zulip):

Hmm... that not how I understand it

Wesley Wiser (Jun 03 2019 at 11:54, on Zulip):

so basically what I thought was the situation is that Operand::Immediate refers to the memory, e.g. where the 5 in 5 as u8 is stored

Wait do you mean Immediate or Indirect?

Wesley Wiser (Jun 03 2019 at 11:54, on Zulip):

Immediate is just a Scalar. I don't think there's a memory location associated with that.

Wesley Wiser (Jun 03 2019 at 11:54, on Zulip):

(Right?)

oli (Jun 03 2019 at 11:55, on Zulip):

oh, sorry, I meant Indirect

Wesley Wiser (Jun 03 2019 at 11:55, on Zulip):

Ok.

oli (Jun 03 2019 at 11:55, on Zulip):

yea, Operand::Immediate just contains an Immediate, which can only be Scalar or ScalarPair

Wesley Wiser (Jun 03 2019 at 11:56, on Zulip):

So, as I understand it, 5 is a Immediate(Scalar(5)). 5 as u8 runs the code which handles Rvalue::Cast. That code will allocate space for the result so 5 as u8 becomes an Immediate(Pointer(xxxx)) Indirect which points to that allocated memory.

Wesley Wiser (Jun 03 2019 at 11:57, on Zulip):

So prior to this change, we could handle 5 as u8 but because the result became an Immediate, Indirect we couldn't go any further. Thus, we couldn't handle (5 as u8) + 1

Wesley Wiser (Jun 03 2019 at 11:57, on Zulip):

Now we can and we get the correct 6u8.

Wesley Wiser (Jun 03 2019 at 12:00, on Zulip):

So going back to your comment, 1 matches my understanding.

Wesley Wiser (Jun 03 2019 at 12:01, on Zulip):

In 2, the Immediate is just the pointer to the allocated memory, not the value itself.

Wesley Wiser (Jun 03 2019 at 12:01, on Zulip):

3 matches

oli (Jun 03 2019 at 12:01, on Zulip):

wait

Wesley Wiser (Jun 03 2019 at 12:01, on Zulip):

In 4, the value is a pointer, so that always(?) works

oli (Jun 03 2019 at 12:01, on Zulip):

are we confusing Immediate and Indirect again?

Wesley Wiser (Jun 03 2019 at 12:02, on Zulip):

hmmm

Wesley Wiser (Jun 03 2019 at 12:02, on Zulip):

Yes

Wesley Wiser (Jun 03 2019 at 12:02, on Zulip):

So prior to this change, we could handle 5 as u8 but because the result became an Immediate, we couldn't go any further. Thus, we couldn't handle (5 as u8) + 1

should be:

So prior to this change, we could handle 5 as u8 but because the result became an Indirect, we couldn't go any further. Thus, we couldn't handle (5 as u8) + 1

oli (Jun 03 2019 at 12:03, on Zulip):

So, as I understand it, 5 is a Immediate(Scalar(5)). 5 as u8 runs the code which handles Rvalue::Cast. That code will allocate space for the result so 5 as u8 becomes an Immediate(Pointer(xxxx)) which points to that allocated memory.

Also: 5 as u8 becomes Indirect(MPlace { ptr: Pointer(xxx) })

Wesley Wiser (Jun 03 2019 at 12:03, on Zulip):

Yes

oli (Jun 03 2019 at 12:04, on Zulip):

So in step 2, when calling read_immediate, we end up with an Immediate::Scalar(5)

oli (Jun 03 2019 at 12:04, on Zulip):

(I think?)

oli (Jun 03 2019 at 12:05, on Zulip):

The source of read_immediate is https://github.com/rust-lang/rust/blob/627486af15d222bcba336b12ea92a05237cc9ab1/src/librustc_mir/interpret/operand.rs#L271

oli (Jun 03 2019 at 12:06, on Zulip):

and try_as_mplace gives us an MPlace if we have Operand::Indirect: https://github.com/rust-lang/rust/blob/627486af15d222bcba336b12ea92a05237cc9ab1/src/librustc_mir/interpret/place.rs#L235

Wesley Wiser (Jun 03 2019 at 12:07, on Zulip):

I thought we ended up with a Immediate::Scalar(ptr) or whatever. Basically the equivalent of int* x = (int*)malloc(...); and then the raw address x.

oli (Jun 03 2019 at 12:07, on Zulip):

I'm fairly certain we don't: https://github.com/rust-lang/rust/blob/627486af15d222bcba336b12ea92a05237cc9ab1/src/librustc_mir/interpret/operand.rs#L240 but I have messed up indirect/direct before

oli (Jun 03 2019 at 12:08, on Zulip):

it's more of a int& x = y; where the implementation chooses to represent c++ references as pointers and autoderef for the user

Wesley Wiser (Jun 03 2019 at 12:09, on Zulip):

I believe you are correct

Wesley Wiser (Jun 03 2019 at 12:10, on Zulip):

So then 4 & 5 shouldn't work at all

Wesley Wiser (Jun 03 2019 at 12:10, on Zulip):

But they are...

oli (Jun 03 2019 at 12:11, on Zulip):

yea, that's what baffles me :D

Wesley Wiser (Jun 03 2019 at 12:12, on Zulip):

I'm adding some tracing code so I can see exactly what all these values are

Wesley Wiser (Jun 03 2019 at 12:13, on Zulip):

The issue has got be with our understanding of 2

Wesley Wiser (Jun 03 2019 at 12:14, on Zulip):

If it works they way we're saying it does, then 4 & 5 don't make any sense at all

oli (Jun 03 2019 at 12:14, on Zulip):

yea, I'm reviewing 4 and 5 rn

Wesley Wiser (Jun 03 2019 at 12:14, on Zulip):

and yet they work. So they have to be doing something correctly

oli (Jun 03 2019 at 12:14, on Zulip):

maybe we missed something there

oli (Jun 03 2019 at 12:15, on Zulip):

OR: we are somehow reading from ByRef wrongly elsewhere

Wesley Wiser (Jun 03 2019 at 12:17, on Zulip):

But operand_from_ref does this:

let ptr = imm_ty.to_scalar_ptr().ok()?.to_ptr().ok()?;

let allocation = self.ecx.memory.get(ptr.alloc_id).ok()?.clone();
oli (Jun 03 2019 at 12:17, on Zulip):

maybe that code never goes beyond the second ?

Wesley Wiser (Jun 03 2019 at 12:17, on Zulip):

How is that working?

oli (Jun 03 2019 at 12:17, on Zulip):

and your test already works :D

Wesley Wiser (Jun 03 2019 at 12:18, on Zulip):

If it doesn't, then we don't do this https://github.com/rust-lang/rust/pull/61437/files#diff-9e103702275cbef342c2decd3395bf3bR599

oli (Jun 03 2019 at 12:18, on Zulip):

yea, but the test you wrote may just never excercise Indirect

Wesley Wiser (Jun 03 2019 at 12:19, on Zulip):
TRACE 2019-06-03T12:18:57Z: rustc_mir::transform::const_prop: handling indirect
TRACE 2019-06-03T12:18:57Z: rustc_mir::transform::const_prop: reading value: OpTy { op: Indirect(MemPlace { ptr: AllocId(2).0x0, align: Align { pow2: 0 }, meta: None }), layout: TyLayout { ty: u8, details: LayoutDetails { variants: Single { index: 0 }, fields: Union(0), abi: Scalar(Scalar { value: Int(I8, false), valid_range: 0..=255 }), align: AbiAndPrefAlign { abi: Align { pow2: 0 }, pref: Align { pow2: 0 } }, size: Size { raw: 1 } } } }
TRACE 2019-06-03T12:18:57Z: rustc_mir::transform::const_prop: imm: Some(ImmTy { imm: Scalar(0x02), layout: TyLayout { ty: u8, details: LayoutDetails { variants: Single { index: 0 }, fields: Union(0), abi: Scalar(Scalar { value: Int(I8, false), valid_range: 0..=255 }), align: AbiAndPrefAlign { abi: Align { pow2: 0 }, pref: Align { pow2: 0 } }, size: Size { raw: 1 } } } })
TRACE 2019-06-03T12:18:57Z: rustc_mir::transform::const_prop: imm_ty: ImmTy { imm: Scalar(0x02), layout: TyLayout { ty: u8, details: LayoutDetails { variants: Single { index: 0 }, fields: Union(0), abi: Scalar(Scalar { value: Int(I8, false), valid_range: 0..=255 }), align: AbiAndPrefAlign { abi: Align { pow2: 0 }, pref: Align { pow2: 0 } }, size: Size { raw: 1 } } } }
oli (Jun 03 2019 at 12:19, on Zulip):

right, but what I mean is that the binop also works: https://github.com/rust-lang/rust/blob/6901ad4c424dc515cfa0101591564ce14f61788c/src/librustc_mir/transform/const_prop.rs#L478

Wesley Wiser (Jun 03 2019 at 12:19, on Zulip):

maybe that code never goes beyond the second ?

I think you hit the nail on the head here

oli (Jun 03 2019 at 12:20, on Zulip):

because it does read_immediate

Wesley Wiser (Jun 03 2019 at 12:20, on Zulip):

That's where the logging I added stops

oli (Jun 03 2019 at 12:20, on Zulip):

still weird that the test shows what it does

Wesley Wiser (Jun 03 2019 at 12:20, on Zulip):

Yes, that already worked

Wesley Wiser (Jun 03 2019 at 12:20, on Zulip):

You were right

Wesley Wiser (Jun 03 2019 at 12:21, on Zulip):

So that's why the test is working

oli (Jun 03 2019 at 12:21, on Zulip):

oh

oli (Jun 03 2019 at 12:21, on Zulip):

wait

oli (Jun 03 2019 at 12:21, on Zulip):

the test is indeed showing it: _2 = const 2u32 as u8 (Misc);

oli (Jun 03 2019 at 12:21, on Zulip):

after optimizations that cast should be gone

Wesley Wiser (Jun 03 2019 at 12:21, on Zulip):

Yeah

oli (Jun 03 2019 at 12:21, on Zulip):

:face_palm:

Wesley Wiser (Jun 03 2019 at 12:22, on Zulip):

So read_immediate() is doing the deref for us

oli (Jun 03 2019 at 12:22, on Zulip):

well "deref"

Wesley Wiser (Jun 03 2019 at 12:22, on Zulip):

heh yeah

oli (Jun 03 2019 at 12:22, on Zulip):

not semantically, but implementation wise, yes

oli (Jun 03 2019 at 12:23, on Zulip):

ok, so since read_immediate "just works", I suggest to throw away all changes of the PR and just do try_read_immediate instead of matching on the Operand, that way we end up with an ImmTy that we can match on and get either a scalar or a scalar pair out of

oli (Jun 03 2019 at 12:24, on Zulip):

or, if we want to const propagate even non-immediates, we go by the ByRef route

oli (Jun 03 2019 at 12:25, on Zulip):

but as already mentioned on the PR, that may overeagerly intern allocations

oli (Jun 03 2019 at 12:25, on Zulip):

so that should probably only be done at higher optimizations levels

Wesley Wiser (Jun 03 2019 at 12:25, on Zulip):

We still need to handle Immediate ourselves right? Since try_read_immediate does a try_as_mplace which fails if we're an Immediate

oli (Jun 03 2019 at 12:26, on Zulip):

it gives you the Immediate if try_as_mplace returns Err

oli (Jun 03 2019 at 12:27, on Zulip):

all very confusing :D I agree

oli (Jun 03 2019 at 12:27, on Zulip):

failure is success

Wesley Wiser (Jun 03 2019 at 12:27, on Zulip):

Oh https://github.com/rust-lang/rust/blob/627486af15d222bcba336b12ea92a05237cc9ab1/src/librustc_mir/interpret/operand.rs#L283

Wesley Wiser (Jun 03 2019 at 12:27, on Zulip):

Ok, yeah we can just call that

Wesley Wiser (Jun 03 2019 at 12:29, on Zulip):

Thanks! I think @Christian Poveda and I can take it from there

Christian Poveda (Jun 03 2019 at 13:40, on Zulip):

Why all the fun happens when I'm sleeping?

Christian Poveda (Jun 03 2019 at 13:46, on Zulip):

So, then we just need to take the OpTy and use try_read_immediate?

oli (Jun 03 2019 at 13:47, on Zulip):

depends... just emitting a ByRef constant for the Indirect is more general

oli (Jun 03 2019 at 13:48, on Zulip):

but requires more memory

oli (Jun 03 2019 at 13:48, on Zulip):

I think you don't actually need to change anything here, because propagation works even over "unpropagated" locals

oli (Jun 03 2019 at 13:49, on Zulip):

it's just a question of whether the Indirect should show up in the MIR

oli (Jun 03 2019 at 13:49, on Zulip):

the propagation happens irrelevant of whether you modify the MIR

Christian Poveda (Jun 03 2019 at 13:56, on Zulip):

Let me see if I understand correctly, the thing is that in fact evaluating Indirects would use too much memory, is that right?

oli (Jun 03 2019 at 14:08, on Zulip):

evaluating e.g. an as cast will allocate additional memory for the duration of the const prop pass. That memory will get cleared afterwards. If you create a ty::Const though, the memory will need to get interned first (which you did), and then it will stay around for the entire compilation.

oli (Jun 03 2019 at 14:09, on Zulip):

if we are never using that value though (as we don't in the given example, because the cast's result is dead code after the propagation), then that would just be a waste of memory

oli (Jun 03 2019 at 14:09, on Zulip):

If the value didn't get promoted or has more use sites later, then that memory usage is ok

oli (Jun 03 2019 at 14:10, on Zulip):

if the value itself is a scalar, then we should not be using ByRef anyway, so... we'd need to do the try_read_immediate optimization

oli (Jun 03 2019 at 14:11, on Zulip):

So... I guess do try_read_immediate for now and leave a fixme to also handle the situation where try_read_immediate fails

Christian Poveda (Jun 03 2019 at 14:12, on Zulip):

evaluating e.g. an as cast will allocate additional memory for the duration of the const prop pass. That memory will get cleared afterwards. If you create a ty::Const though, the memory will need to get interned first (which you did), and then it will stay around for the entire compilation.

Ohh ok I understand

Christian Poveda (Jun 03 2019 at 14:18, on Zulip):

So... I guess do try_read_immediate for now and leave a fixme to also handle the situation where try_read_immediate fails

here https://github.com/rust-lang/rust/pull/61437/files#diff-9e103702275cbef342c2decd3395bf3bR594, right?

oli (Jun 03 2019 at 14:22, on Zulip):

No, get rid of the entire match value and do let imm = ....try_immediate(value).ok()??; or sth and just have the match imm that is currently in the Operand::Immediate arm

Christian Poveda (Jun 03 2019 at 14:27, on Zulip):

so something like this:

                // FIXME> figure out what tho do when try_read_immediate fails
                let imm = self.use_ecx(source_info, |this| {
                    this.ecx.try_read_immediate(value)
                });

                if let Some(imm) = imm {
                    if let Some(op) = self.operand_from_ref(imm, source_info.span) {
                        *rval = Rvalue::Use(op);
                    }
                }
oli (Jun 03 2019 at 14:28, on Zulip):

no, I mean, don't have an match value (and thus Operand::Indirect) arm at all

Christian Poveda (Jun 03 2019 at 14:28, on Zulip):

yes yes

oli (Jun 03 2019 at 14:29, on Zulip):

/me is confused

Christian Poveda (Jun 03 2019 at 14:29, on Zulip):

me too, me too

Christian Poveda (Jun 03 2019 at 14:29, on Zulip):

hahaha

Christian Poveda (Jun 03 2019 at 14:35, on Zulip):

no, I mean, don't have an match value (and thus Operand::Indirect) arm at all

that code would be the replacement for the whole thing

oli (Jun 03 2019 at 14:36, on Zulip):

ah, no don't use operand_from_ref, use the existing match that emits Scalar and ScalarPair

Christian Poveda (Jun 03 2019 at 14:37, on Zulip):

No, get rid of the entire match value and do let imm = ....try_immediate(value).ok()??; or sth and just have the match imm that is currently in the Operand::Immediate arm

this is basically taking the Operand::Immediate arm and deleting the match value

Christian Poveda (Jun 03 2019 at 14:37, on Zulip):

but doing try_read_immediate instead of read_immediate

Christian Poveda (Jun 03 2019 at 14:38, on Zulip):

ohhhhhhhh

Christian Poveda (Jun 03 2019 at 14:38, on Zulip):

i get it, i feel dumb

Christian Poveda (Jun 03 2019 at 14:47, on Zulip):
        // FIXME> figure out what tho do when try_read_immediate fails
        let imm = self.use_ecx(source_info, |this| {
            this.ecx.try_read_immediate(value)
        });

        if let Some(imm) = imm {
            match imm {
                interpret::Immediate::Scalar(ScalarMaybeUndef::Scalar(scalar)) => {
                    *rval = Rvalue::Use(
                        self.operand_from_scalar(scalar, value.layout.ty, source_info.span));
                },
                Immediate::ScalarPair(
                    ScalarMaybeUndef::Scalar(one),
                    ScalarMaybeUndef::Scalar(two)
                    ) => {
                    let ty = &value.layout.ty.sty;
                    if let ty::Tuple(substs) = ty {
                        *rval = Rvalue::Aggregate(
                            Box::new(AggregateKind::Tuple),
                            vec![
                            self.operand_from_scalar(
                                one, substs[0].expect_ty(), source_info.span
                                ),
                                self.operand_from_scalar(
                                    two, substs[1].expect_ty(), source_info.span
                                    ),
                            ],
                            );
                    }
                },
                _ => { }

            }
        }
Christian Poveda (Jun 03 2019 at 14:51, on Zulip):

great, I'm gonna run tests

Christian Poveda (Jun 03 2019 at 14:56, on Zulip):

try_read_immediate is private

Christian Poveda (Jun 03 2019 at 14:56, on Zulip):

should I make it public?

oli (Jun 03 2019 at 14:56, on Zulip):

jop

oli (Jun 03 2019 at 14:56, on Zulip):

crate wide only please

Christian Poveda (Jun 03 2019 at 14:56, on Zulip):

crate wide?

oli (Jun 03 2019 at 14:57, on Zulip):

pub(crate)

Christian Poveda (Jun 03 2019 at 14:57, on Zulip):

oh

Christian Poveda (Jun 03 2019 at 14:57, on Zulip):

sure

Christian Poveda (Jun 03 2019 at 15:07, on Zulip):

I'm not using the PlaceTy in replace_with_const, is that ok?

Wesley Wiser (Jun 03 2019 at 15:08, on Zulip):

Yeah, I added that in 499d1178181a428dccac3d24a9314f68c3f1cc58

Wesley Wiser (Jun 03 2019 at 15:08, on Zulip):

But if you don't need it, you can just remove it

Christian Poveda (Jun 03 2019 at 15:34, on Zulip):

This is taking ages. I've never built rust on this machine hahah

Wesley Wiser (Jun 03 2019 at 15:35, on Zulip):

For most tests, you can do ./x.py test --stage 1 -i src/test/{test-dir}

Wesley Wiser (Jun 03 2019 at 15:36, on Zulip):

And for building just the compiler ./x.py build --stage 1 -i src/libstd

Christian Poveda (Jun 03 2019 at 15:36, on Zulip):

For most tests, you can do ./x.py test --stage 1 -i src/test/{test-dir}

yeah I'm doing this

Christian Poveda (Jun 03 2019 at 15:36, on Zulip):

ty

Christian Poveda (Jun 03 2019 at 16:42, on Zulip):

ok we're ready

Christian Poveda (Jun 03 2019 at 18:53, on Zulip):

the same test failed i think

Christian Poveda (Jun 03 2019 at 18:57, on Zulip):

I'll fix the formatting issues just before rebasing

Wesley Wiser (Jun 03 2019 at 19:05, on Zulip):

@Christian Poveda I think the tests just need to be updated actually.

Wesley Wiser (Jun 03 2019 at 19:05, on Zulip):

They're failing because your patch is working now!

Christian Poveda (Jun 03 2019 at 19:08, on Zulip):

Oh yea, _3 and _4 have the actual value

Christian Poveda (Jun 03 2019 at 19:09, on Zulip):

shouldnt that pointer change from run to run?

Wesley Wiser (Jun 03 2019 at 19:11, on Zulip):

In mir-opt/const_prop/const_prop_fails_gracefully.rs?

Wesley Wiser (Jun 03 2019 at 19:11, on Zulip):

Yeah, I'm not sure about that

Wesley Wiser (Jun 03 2019 at 19:11, on Zulip):

That's a question for @oli

Wesley Wiser (Jun 03 2019 at 19:11, on Zulip):

mir-opt/const_prop/indirect.rs Just needs to be updated though. The new behavior is correct.

Christian Poveda (Jun 03 2019 at 20:18, on Zulip):

when Oli answers I'll upload both tests

Wesley Wiser (Jun 03 2019 at 20:22, on Zulip):

Sounds good

Wesley Wiser (Jun 03 2019 at 20:24, on Zulip):

We'll probably need to rewrite the const_prop_fails_gracefully test because the point of the test it to make sure the compiler doesn't ICE when it can't const prop stuff and you've implemented support for the thing it previously couldn't const prop.

However, your question about the pointer leaking into the MIR should still be answered and we should have a test around whatever the correct behavior there is as well.

oli (Jun 03 2019 at 23:02, on Zulip):

indirect.rs just needs updating, I agree

oli (Jun 03 2019 at 23:03, on Zulip):

const_prop_fails_gracefully is pretty cool, since it actually shows that we don't propagate _1 = move _2 as usize (Misc);, because _2 is a pointer which is not a valid usize at compile-time

oli (Jun 03 2019 at 23:04, on Zulip):

we won't ever const prop that, so the test still properly excercises what it is supposed to

oli (Jun 03 2019 at 23:05, on Zulip):

The AllocId showing up in MIR is a little funky, but ok. We should probably record all AllocIds printed during MIR printing and print their memory, too, but that's a future thing to do (open an issue about it?)

Christian Poveda (Jun 04 2019 at 00:53, on Zulip):

But then should i change the test and hide the AllocIds? or should I leave them explicitly as in

[01:09:11]         _4 = const Scalar(AllocId(1).0x0) : &i32;
[01:09:11]         _3 = const Scalar(AllocId(1).0x0) : &i32;
[01:09:11]         _2 = const Scalar(AllocId(1).0x0) : *const i32;
Wesley Wiser (Jun 04 2019 at 00:57, on Zulip):

Leave them in I think

Christian Poveda (Jun 04 2019 at 01:00, on Zulip):

sure, I'll fix that and commit

Christian Poveda (Jun 04 2019 at 01:03, on Zulip):

I'm testing to avoid wasting CI :P

Wesley Wiser (Jun 04 2019 at 01:08, on Zulip):

The infra team thanks you :bow:

Christian Poveda (Jun 04 2019 at 01:18, on Zulip):

Well its good to keep the pc exercising :P

Christian Poveda (Jun 04 2019 at 01:20, on Zulip):

idk what to do with vim identation, I'll think I'll have to fix the identation manually

centril (Jun 04 2019 at 01:30, on Zulip):

I'm testing to avoid wasting CI :P

@Christian Poveda I'd much rather you save your own time because human time is much more precious than CI time. :slight_smile:

Christian Poveda (Jun 04 2019 at 01:31, on Zulip):

oh well if you insist

Christian Poveda (Jun 04 2019 at 04:52, on Zulip):

Ok it is done, going to rebase

Last update: Nov 17 2019 at 08:05UTC