Cleanup the shim code #47865

Manishearth · 2018-01-30T05:22:28Z

We now write directly to RETURN_PLACE instead of creating intermediates
tuple_like_shim takes an iterator (used by [WIP] Make derive(Clone) memcpy on enum variants containing only Copy types #47867)
tuple_like_shim no longer relies on it being the first thing to create blocks, and uses relative block indexing in a cleaner way (necessary for [WIP] Make derive(Clone) memcpy on enum variants containing only Copy types #47867)
All the shim builders take dest, src arguments instead of hardcoding RETURN_PLACE

Manishearth · 2018-01-30T06:52:14Z

This is a precursor to my patches for #47796 (which may need an rfc). @eddyb noticed that the tuple shim code was suboptimal and since I'm fixing that anyway might as well get these cleanups through.

eddyb · 2018-01-30T07:51:50Z

src/librustc_mir/shim.rs

-        cleanup: BasicBlock
-    ) -> Place<'tcx> {
+        cleanup: BasicBlock,
+        place: Place<'tcx>


Can you name this dest?

eddyb · 2018-01-30T08:23:31Z

src/librustc/mir/mod.rs

@@ -1520,6 +1520,9 @@ pub enum AggregateKind<'tcx> {
    /// active field number and is present only for union expressions
    /// -- e.g. for a union expression `SomeUnion { c: .. }`, the
    /// active field index would identity the field `c`
+    ///
+    /// For enums, the second field is the index of the variant
+    /// within AdtDef::fields


Isn't this also 5 lines above?

The confusion was "is it 0-indexed variant index, 1-indexed variant index, or the discriminant value". Basically, the fact that it says that it's zero for structs and unions suggests that it's perhaps nonzero for enums, and it's better if folks don't need to verify this.

You can just edit that part instead of adding more elsewhere... also you wrote AdtDef::fields instead of variants. And this shouldn't be in this PR anyway.

Actually structs and unions have exactly one variant so the rephrasing can be even more enum-focused.

Manishearth · 2018-01-30T09:45:36Z

fixed

eddyb · 2018-01-30T11:50:48Z

src/librustc/mir/mod.rs

+    /// The second field is variant number (discriminant),
+    /// i.e. the index of the variant within AdtDef::variants.
+    ///
+    /// It's equal to 0 for struct and union expressions. The fourth field is


The newline seems misplaced. Also, we should avoid using "discriminant", because that's a different thing that may not be equal to the variant index. All the relevant code uses "variant index" (not "variant number").

eddyb · 2018-01-30T11:51:08Z

src/librustc_mir/shim.rs

-        cleanup: BasicBlock
-    ) -> Place<'tcx> {
+        cleanup: BasicBlock,
+        dest: Place<'tcx>


Please put this before rcvr_field and rename that to src.

eddyb · 2018-01-30T11:54:51Z

src/librustc_mir/shim.rs

@@ -540,7 +536,9 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> {
        // `let cloned = Clone::clone(rcvr[beg])`;
        // Goto #3 if ok, #5 if unwinding happens.
        let rcvr_field = rcvr.clone().index(beg);
-        let cloned = self.make_clone_call(ty, rcvr_field, BasicBlock::new(3), BasicBlock::new(5));
+        let cloned = self.make_place(Mutability::Not, ty);


Please pass in ret_field from below as the destination (and rename it to dest_elem - also rcvr_field should be src_elem).

eddyb · 2018-01-30T12:10:57Z

src/librustc_mir/shim.rs

+    /// Gives the index of an upcoming BasicBlock, with an offset.
+    /// offset=0 will give you the index of the next BasicBlock,
+    /// offset=1 will give the index of the next-to-next block,
+    /// offset=-1 will give


This comment is unfinished.

eddyb · 2018-01-30T12:11:37Z

src/librustc_mir/shim.rs

@@ -571,7 +573,7 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> {
        // `return ret;`
        let ret_statement = self.make_statement(
            StatementKind::Assign(
-                Place::Local(RETURN_PLACE),
+                dest,
                Rvalue::Use(Operand::Move(ret.clone())),


Please make this function also write in-place.

eddyb · 2018-01-30T12:11:55Z

src/librustc_mir/shim.rs

-        let rcvr_field = rcvr.clone().index(beg);
-        let cloned = self.make_clone_call(ty, rcvr_field, BasicBlock::new(3), BasicBlock::new(5));
+        let src_field = src.clone().index(beg);
+        let ret_field = ret.clone().index(beg);


Can you name this dest_field and put it before src_field?

eddyb · 2018-01-30T12:12:22Z

src/librustc_mir/shim.rs

-            _ => bug!("only tuples and closures are accepted"),
-        };
+    fn tuple_like_shim<I>(&mut self, dest: Place<'tcx>,
+                          rcvr: Place<'tcx>, tys: I)


This should be named src not rcvr.

Manishearth · 2018-01-30T13:44:31Z

done

eddyb · 2018-01-30T13:54:40Z

src/librustc_mir/shim.rs

            }
+
+            previous_place = Some((place, cleanup_block));


This should be named previous_field, I think.

eddyb · 2018-01-30T14:35:39Z

src/librustc_mir/shim.rs


            // BB #(2i)
-            // `returns[i] = Clone::clone(&rcvr.i);`
+            // `return.i = Clone::clone(&src.i);`


dest.i instead of return.i

eddyb · 2018-01-30T14:36:11Z

cc @arielb1

nikomatsakis

Seems pretty good to me. A few nits/questions.

nikomatsakis · 2018-01-30T22:37:59Z

src/librustc_mir/shim.rs

@@ -294,7 +294,7 @@ fn build_clone_shim<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
 {
    debug!("build_clone_shim(def_id={:?})", def_id);

-    let mut builder = CloneShimBuilder::new(tcx, def_id);
+    let mut builder = CloneShimBuilder::new(tcx, def_id, self_ty);


is there a test for whatever motivated this commit?

Without this commit everything ICEs. Everything.

Any code instantiating a clone shim of tuples or arrays or closures (libcore does) will break because the type used will be TySelf (not TyArray or TyClosure or whatever) and we'll get errors because we're trying to call .index() or .field() on a place that can't be indexed. I can leave a comment to this effect somewhere.

I mean I can see why this is wrong, but I'm a bit confused -- the code works now, afaik, right? Or is this just patching up some earlier commit of yours?

It breaks once the change to the array shim is made - anything that depends on the type (e.g. indexing or enum discriminants) starts breaking because it'd see an unsubstituted Self.

Thsi commit should be earlier in the series I think.

We basically don't hit it because we write to temporary Places and then copy and that works well. I don't recall the exact details, but eddyb can explain.

ok good enough

Oh right yeah this doesn't break the tuple stuff but it does break the array stuff (and the enum stuff from my other PR)

nikomatsakis · 2018-01-30T22:38:09Z

src/librustc_mir/shim.rs

@@ -327,8 +327,10 @@ struct CloneShimBuilder<'a, 'tcx: 'a> {
 }

 impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> {
-    fn new(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> Self {
-        let sig = tcx.fn_sig(def_id);
+    fn new(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId,


Nit: can we use rustfmt-style formatting here?

nikomatsakis · 2018-01-30T22:41:00Z

src/librustc_mir/shim.rs

@@ -501,11 +501,11 @@ impl<'a, 'tcx> CloneShimBuilder<'a, 'tcx> {
    fn array_shim(&mut self, ty: Ty<'tcx>, len: u64) {
        let tcx = self.tcx;
        let span = self.span;
-        let rcvr = Place::Local(Local::new(1+0)).deref();
+        let src = Place::Local(Local::new(1+0)).deref();
+        let dest = Place::Local(RETURN_PLACE);


Hmm, I think this is ok, but it's interesting that we can observe the mutations even on unwind etc. Do you know off hand @eddyb if this is already true? Seems like something that we will eventually want to be legal, though. Effectively you're giving in an &mut to the (uninitialized) RETURN_PLACE, so we can do what we want there.

But I feel like this has been an interesting question at times in the UCG discussions (when precisely can we reuse places etc).

Otherwise we get random TySelfs there, which means operations on RETURN_PLACE end up breaking down badly.

Manishearth · 2018-02-01T02:39:33Z

done

nikomatsakis · 2018-02-01T18:22:41Z

@bors r+

bors · 2018-02-01T18:22:42Z

📌 Commit 8a8f91b has been approved by nikomatsakis

bors · 2018-02-05T01:45:52Z

⌛ Testing commit 8a8f91b with merge dd29d3d...

@eddyb

Cleanup the shim code - We now write directly to `RETURN_PLACE` instead of creating intermediates - `tuple_like_shim` takes an iterator (used by #47867) - `tuple_like_shim` no longer relies on it being the first thing to create blocks, and uses relative block indexing in a cleaner way (necessary for #47867) - All the shim builders take `dest, src` arguments instead of hardcoding RETURN_PLACE r? @eddyb

bors · 2018-02-05T04:32:01Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing dd29d3d to master...

rust-highfive assigned eddyb Jan 30, 2018

Manishearth force-pushed the cleanup-shim branch 3 times, most recently from d658161 to 973881d Compare January 30, 2018 06:26

rust-highfive assigned nikomatsakis and unassigned eddyb Jan 30, 2018