Move MIR towards a single kind of local #36752

jonas-schievink · 2016-09-26T18:49:19Z

This PR modifies MIR to handle function arguments (Arg), user-defined variable bindings (Var), compiler-generated temporaries (Tmp), as well as the return value pointer equally. All of them are replaced with a single Local type, a few functions for iterating over different kinds of locals, and a way to get the kind of local we're dealing with (mainly used in the constant qualification/propagation passes).

~~I haven't managed to fix one remaining issue: A StorageDead not getting emitted for a variable (see the TODO in the test). If that's fixed, this is basically good to go.~~ Found the issue (an off-by-one error), fix incoming.

r? @eddyb for changes to constant qualification and propagation I'm not quite sure about

If MIR is for a "rust-call" ABI function, the last arg would always have `spread` set to `true`. Move this flag into `Mir` instead.

eddyb

LGTM modulo nits. cc @rust-lang/compiler In case anyone has anything to say against it.

eddyb · 2016-09-26T19:47:43Z

src/librustc/mir/repr.rs

+    /// is passed as its individual components at the LLVM level.
+    ///
+    /// This is used for the "rust-call" ABI.
+    pub spread_last_arg: bool,


I'd rather have an Option<Local> here instead, special-casing the last argument is a step backwards for VG.

eddyb · 2016-09-26T19:56:35Z

src/librustc/mir/repr.rs

+    /// Returns an iterator over all function arguments.
+    #[inline]
+    pub fn arg_iter<'a>(&'a self) -> impl Iterator<Item=Local> + 'a {
+        (1..self.arg_count+1).map(Local::new)


This doesn't need to borrow self.

eddyb · 2016-09-26T19:57:14Z

src/librustc/mir/repr.rs

+
+    /// Returns an iterator over all temporaries.
+    #[inline]
+    pub fn temp_iter<'a>(&'a self) -> impl Iterator<Item=Local> + 'a {


The usual conversion would be temps, I believe (same for the ones below).

eddyb · 2016-09-26T19:59:08Z

src/librustc_borrowck/borrowck/mir/dataflow/sanity_check.rs

-        }
-        repr::Operand::Consume(_) |
+        repr::Operand::Consume(ref lval) => match *lval {
+             repr::Lvalue::Local(local) if mir.local_kind(local) == LocalKind::Temp => {


Don't think this code should care. cc @pnkfelix

eddyb · 2016-09-26T20:07:44Z

src/librustc_mir/def_use.rs

-            Lvalue::Var(var) => Some(Local::new(var.index() + self.arg_count)),
-            Lvalue::Temp(temp) => {
-                Some(Local::new(temp.index() + self.arg_count + self.var_count))
+        if let Lvalue::Local(local) = *lvalue {


Sweet! I'm sorry @pcwalton had to write this in the first place, but, this is all in hindsight.

eddyb · 2016-09-26T20:10:55Z

src/librustc_mir/transform/promote_consts.rs

            block: BasicBlock::new(0),
            statement_index: usize::MAX
        });
-        self.assign(Lvalue::ReturnPointer, rvalue, span);
+
+        self.assign(Lvalue::Local(RETURN_POINTER), rvalue, span);


Might be able to change assign to take a Local.

eddyb · 2016-09-26T20:13:13Z

src/librustc_trans/mir/mod.rs

+        let retptr = allocate_local(mir::RETURN_POINTER);
+        iter::once(retptr)
+            .chain(args.into_iter())
+            .chain(mir.var_and_temp_iter().map(&mut allocate_local))


Why not move allocate_local into the map call?

This has gone through a refactoring, I'll remove the &mut.

eddyb · 2016-09-26T20:13:55Z

src/test/mir-opt/deaggregator_test.rs

-//     return = Baz { x: tmp0, y: const F32(0), z: const false }; // scope ...
+//     local2 = local1;                     // scope 0 at main.rs:8:8: 8:9
+//     local3 = local2;                     // scope 1 at main.rs:9:14: 9:15
+//     local0 = Baz { x: local3, y: const F32(0), z: const false }; // scope ...


Losing the names is unfortunate, do you think we could recover them?

Do you mean what kind of local each one is? We could recover that, but the full MIR dump already contains variable names so you can differentiate temps and variables. The full dump also has a comment on local0 explaining that it's the return pointer.

It's harder to read though. Maybe if it was %5 like in LLVM or _5, it'd be easier. But still.
I guess there's no easy way to recover that information due to how it's printed.

eddyb · 2016-09-26T22:29:54Z

src/librustc/mir/repr.rs

+    /// Returns an iterator over all user-defined variables and compiler-generated temporaries (all
+    /// locals that are neither arguments nor the return pointer).
+    #[inline]
+    pub fn vars_and_temps_iter<'a>(&'a self) -> impl Iterator<Item=Local> + 'a {


This also doesn't need 'a.

eddyb · 2016-09-26T22:53:02Z

src/librustc_trans/mir/mod.rs

-                    // they are the two sub-fields of a single aggregate field.
-                    let meta = &fcx.fn_ty.args[idx];
+        if let Some(spread_local) = mir.spread_arg {
+            if local == spread_local {


You can write this as if Some(local) == mir.spread_arg.

solson · 2016-09-28T21:24:40Z

LGTM, but the loss of names in the MIR is somewhat annoying. If we can't easily recover the original names, then since all locals are now printed the same, I'd prefer to make the prefix short, like @eddyb's _5 suggestion.

jonas-schievink · 2016-09-28T22:12:54Z

We could recover the local kind (by threading a &Mir through the printer), but I actually like this way better. I also don't know how we should print this information without confusing the reader. Just replacing local with arg etc. doesn't seem appropriate since it suggests that arguments, variables and temps are still different things. Adding it to the comment might be worth a shot, but comments aren't placed on arguments.

For the record, here's a "full" MIR dump for a small test program after this patch:

fn testroutine(local1: u8, local2: u8) -> u8 {
    let mut local0: u8;                  // return pointer
    scope 1 {
        let local3: u8;                  // "start" in scope 1 at test.rs:1:16: 1:21
        let local4: u8;                  // "end" in scope 1 at test.rs:1:27: 1:30
    }
    let mut local5: std::iter::Map<std::ops::Range<u8>, [closure@test.rs:2:24: 2:33]>;
    let mut local6: std::ops::Range<u8>;
    let mut local7: u8;
    let mut local8: u8;
    let mut local9: u8;
    let mut local10: u8;
    let mut local11: (u8, bool);
    let mut local12: [closure@test.rs:2:24: 2:33];

    bb0: {
        StorageLive(local3);             // scope 0 at test.rs:1:16: 1:21
        local3 = local1;                 // scope 0 at test.rs:1:16: 1:21
        StorageLive(local4);             // scope 0 at test.rs:1:27: 1:30
        local4 = local2;                 // scope 0 at test.rs:1:27: 1:30
        StorageLive(local5);             // scope 1 at test.rs:2:5: 2:34
        StorageLive(local6);             // scope 1 at test.rs:2:5: 2:19
        StorageLive(local7);             // scope 1 at test.rs:2:6: 2:11
        StorageLive(local8);             // scope 1 at test.rs:2:6: 2:11
        local8 = local3;                 // scope 1 at test.rs:2:6: 2:11
        local7 = local8;                 // scope 1 at test.rs:2:6: 2:11
        StorageLive(local9);             // scope 1 at test.rs:2:13: 2:18
        StorageLive(local10);            // scope 1 at test.rs:2:13: 2:16
        local10 = local4;                // scope 1 at test.rs:2:13: 2:16
        local11 = CheckedAdd(local10, const 1u8); // scope 1 at test.rs:2:13: 2:18
        assert(!(local11.1: bool), "attempt to add with overflow") -> bb1; // scope 1 at test.rs:2:13: 2:18
    }

    bb1: {
        local9 = (local11.0: u8);        // scope 1 at test.rs:2:13: 2:18
        local6 = std::ops::Range::<u8> { start: local7, end: local9 }; // scope 1 at test.rs:2:6: 2:18
        StorageLive(local12);            // scope 1 at test.rs:2:24: 2:33
        local12 = [closure@test.rs:2:24: 2:33]; // scope 1 at test.rs:2:24: 2:33
        local5 = <std::ops::Range<u8> as std::iter::Iterator>::map::<u8, [closure@test.rs:2:24: 2:33]>(local6, local12) -> bb2; // scope 1 at test.rs:2:5: 2:34
    }

    bb2: {
        local0 = <std::iter::Map<std::ops::Range<u8>, [closure@test.rs:2:24: 2:33]> as std::iter::Iterator>::sum::<u8>(local5) -> bb3; // scope 1 at test.rs:2:5: 2:40
    }

    bb3: {
        StorageDead(local5);             // scope 1 at test.rs:2:5: 2:34
        StorageDead(local12);            // scope 1 at test.rs:2:24: 2:33
        StorageDead(local6);             // scope 1 at test.rs:2:5: 2:19
        StorageDead(local9);             // scope 1 at test.rs:2:13: 2:18
        StorageDead(local10);            // scope 1 at test.rs:2:13: 2:16
        StorageDead(local7);             // scope 1 at test.rs:2:6: 2:11
        StorageDead(local8);             // scope 1 at test.rs:2:6: 2:11
        StorageDead(local4);             // scope 0 at test.rs:1:27: 1:30
        StorageDead(local3);             // scope 0 at test.rs:1:16: 1:21
        return;                          // scope 1 at test.rs:1:1: 3:2
    }
}

fn testroutine::{{closure}}(local1: &mut [closure@test.rs:2:24: 2:33], local2: u8) -> u8 {
    let mut local0: u8;                  // return pointer
    scope 1 {
        let local3: u8;                  // "i" in scope 1 at test.rs:2:25: 2:26
    }
    let mut local4: u8;
    let mut local5: u8;
    let mut local6: (u8, bool);

    bb0: {
        StorageLive(local3);             // scope 0 at test.rs:2:25: 2:26
        local3 = local2;                 // scope 0 at test.rs:2:25: 2:26
        StorageLive(local4);             // scope 1 at test.rs:2:28: 2:29
        local4 = local3;                 // scope 1 at test.rs:2:28: 2:29
        StorageLive(local5);             // scope 1 at test.rs:2:32: 2:33
        local5 = local3;                 // scope 1 at test.rs:2:32: 2:33
        local6 = CheckedMul(local4, local5); // scope 1 at test.rs:2:28: 2:33
        assert(!(local6.1: bool), "attempt to multiply with overflow") -> bb1; // scope 1 at test.rs:2:28: 2:33
    }

    bb1: {
        local0 = (local6.0: u8);         // scope 1 at test.rs:2:28: 2:33
        StorageDead(local5);             // scope 1 at test.rs:2:32: 2:33
        StorageDead(local4);             // scope 1 at test.rs:2:28: 2:29
        StorageDead(local3);             // scope 0 at test.rs:2:25: 2:26
        return;                          // scope 1 at test.rs:2:24: 2:33
    }
}

fn testroutine(start: u8, end: u8) -> u8 {
    (start..end+1).map(|i| i * i).sum()
}

jonas-schievink · 2016-09-28T22:13:07Z

We could possibly annotate each localN with the local kind, so you'd end up with local0{return}, local1{arg0}, local3{var0 "start"} and local7{tmp3} or something like that.

There's no need for a long prefix, since there's nothing to distinguish anymore.

They're ignored by the test runner, so let's not suggest that they matter

eddyb · 2016-09-28T23:25:38Z

@bors r+

bors · 2016-09-28T23:25:39Z

📌 Commit d2c8893 has been approved by eddyb

bors · 2016-09-29T14:53:34Z

⌛ Testing commit d2c8893 with merge ff67da6...

@eddyb

Move MIR towards a single kind of local This PR modifies MIR to handle function arguments (`Arg`), user-defined variable bindings (`Var`), compiler-generated temporaries (`Tmp`), as well as the return value pointer equally. All of them are replaced with a single `Local` type, a few functions for iterating over different kinds of locals, and a way to get the kind of local we're dealing with (mainly used in the constant qualification/propagation passes). ~~I haven't managed to fix one remaining issue: A `StorageDead` not getting emitted for a variable (see the `TODO` in the test). If that's fixed, this is basically good to go.~~ Found the issue (an off-by-one error), fix incoming. r? @eddyb for changes to constant qualification and propagation I'm not quite sure about

bors · 2016-09-29T18:20:00Z

jonas-schievink added 2 commits September 26, 2016 19:42

Move "rust-call" tuple spreading out of ArgDecl

205dac9

If MIR is for a "rust-call" ABI function, the last arg would always have `spread` set to `true`. Move this flag into `Mir` instead.

[WIP] Move MIR towards a single kind of local

393db2d

rust-highfive assigned eddyb Sep 26, 2016

eddyb suggested changes Sep 26, 2016

View reviewed changes

jonas-schievink added 8 commits September 26, 2016 22:30

Fix off-by-one when emitting StorageDead for vars

e75feea

Remove TODOs (they're done)

894c083

Make spreading use Option<Local>

3b0c318

Rename MIR local iterators to match convention

bcfbdb8

args_iter doesn't need to borrow the MIR

66d2f34

promote_consts: make assign take a Local

dfab092

No need to borrow allocate_local

9ccac2a

Fix tidy

e0249ad

jonas-schievink changed the title ~~[WIP] Move MIR towards a single kind of local~~ Move MIR towards a single kind of local Sep 26, 2016

Ignore local kind in MIR dataflow

a0fa204

eddyb suggested changes Sep 26, 2016

View reviewed changes

jonas-schievink added 2 commits September 27, 2016 02:03

Address review comments

e3293b8

Fix codegen test (value names changed)

168a079

jonas-schievink added 2 commits September 29, 2016 01:11

Change the local prefix to _

34155a8

There's no need for a long prefix, since there's nothing to distinguish anymore.

Remove MIR dump comments from mir-opt tests

d2c8893

They're ignored by the test runner, so let's not suggest that they matter

bors merged commit d2c8893 into rust-lang:master Sep 29, 2016

This was referenced Sep 29, 2016

A way to remove otherwise unused locals from MIR #36306

Merged

[MIR] Initial implementation of inlining #36593

Closed

jonas-schievink deleted the vartmparg branch September 29, 2016 18:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move MIR towards a single kind of local #36752

Move MIR towards a single kind of local #36752

jonas-schievink commented Sep 26, 2016 •

edited

Loading

eddyb left a comment

eddyb Sep 26, 2016 •

edited

Loading

eddyb Sep 26, 2016

eddyb Sep 26, 2016

eddyb Sep 26, 2016

eddyb Sep 26, 2016

eddyb Sep 26, 2016

eddyb Sep 26, 2016

jonas-schievink Sep 26, 2016

eddyb Sep 26, 2016

jonas-schievink Sep 26, 2016

eddyb Sep 26, 2016

eddyb Sep 26, 2016

eddyb Sep 26, 2016

solson commented Sep 28, 2016

jonas-schievink commented Sep 28, 2016

jonas-schievink commented Sep 28, 2016

eddyb commented Sep 28, 2016

bors commented Sep 28, 2016

bors commented Sep 29, 2016

bors commented Sep 29, 2016

Move MIR towards a single kind of local #36752

Move MIR towards a single kind of local #36752

Conversation

jonas-schievink commented Sep 26, 2016 • edited Loading

eddyb left a comment

Choose a reason for hiding this comment

eddyb Sep 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solson commented Sep 28, 2016

jonas-schievink commented Sep 28, 2016

jonas-schievink commented Sep 28, 2016

eddyb commented Sep 28, 2016

bors commented Sep 28, 2016

bors commented Sep 29, 2016

bors commented Sep 29, 2016

jonas-schievink commented Sep 26, 2016 •

edited

Loading

eddyb Sep 26, 2016 •

edited

Loading