[MIR] Reintroduce the unit temporary #30945

nagisa · 2016-01-15T22:51:48Z

As an attempt to make loop body destination be optional, author implemented a pretty self contained
change and deemed it to be (much) uglier than the alternative of just keeping the unit temporary.
Having the temporary created lazily also has a nice property of not figuring in the MIR of
functions which do not use loops of any sort.

r? @nikomatsakis

nagisa · 2016-01-15T22:52:18Z

src/librustc_mir/build/expr/into.rs

+                    // Even if we assign unconditionally, if the loop turns out to be diverging,
+                    // the loop’s “return” value will not appear in the MIR (removed along with the
+                    // exit_block by cfg simplification passes later).
+                    this.cfg.push_assign_unit(exit_block, expr_span, destination);


I have some doubts about suitability of this here. It relies on CFG passes removing the exit_block in order to not introduce ill-typed MIR (see e.g. #30876) in case of loops with diverging types. I’m not sure if having that ill-typedness for a little while in an unreachable block is fine. Any thoughts?

I'm actually a little bit surprised by the behavior of the type checker here. It seems like loop { } has type unit -- but, if you never break out, we consider the loop non-terminating, and hence we allow it to be used in any context. This is probably not how I would want it to work in an ideal world, but it may not be worth trying to change anything here. :)

Therefore, to match the compiler as today, I suspect it should work like this:

if you break out of the loop, then we will push_assign_unit

if you do not, we will not push any assignment at all

Of course we don't know whether you break out of the loop in advance. We could either analyze this, or maybe come back after the fact and check.

The trivial way to check would be to simply check if desination type is () or not. Provided typechecking/inference run before lowering into MIR, like they do now.

It occurs to me we could also add this assignment as an "exiting the scope" action, much like drops. That is, we could record it in the LoopScope, and when we are translating a break, we can insert the store.

I’m not sure if having that ill-typedness for a little while in an unreachable block is fine.

I never answered this directly -- I'd prefer to avoid it, since I plan to be running various type-checking steps on the MIR before we do optimizations and cleanup.

nagisa · 2016-01-15T22:54:55Z

Some resulting MIRs of a few test cases

nagisa · 2016-01-19T17:31:33Z

@nikomatsakis I updated the PR with the approach of storing a flag inside LoopScope.

nikomatsakis · 2016-01-19T18:13:25Z

src/librustc_mir/build/expr/into.rs

-        self.exit_scope(span, loop_scope.extent, block, exit_block);
+        let (exit_block, extent) = {
+            let loop_scope = self.find_loop_scope(span, label);
+            loop_scope.might_terminate = true;


I think this is only true for break, not continue.

@nikomatsakis

Something like

'a: while 1 != 2 { loop { continue 'a } }

is a valid code and essentially terminates the loop inside of which the expression resides?

@nagisa yes, you're right, it depends on where you continue to I suppose

@nagisa seems like we should toggle this flag in exit_scope, not here, and then return the result (as I wrote elsewhere)

nikomatsakis · 2016-01-19T18:16:39Z

@nagisa left some nit-like comments. :)

nagisa · 2016-01-19T19:59:27Z

src/librustc_mir/build/scope.rs

+
+        // We merge iterators of regular scopes and loop scopes based on their extents. This code
+        // relies on the assumption that every loop scope will have an extent of some non-popped
+        // scope.


I hope this comment is a good enough explanation for the overly-smart code below.

This is some awfully clever code, I hope it's correct :) (I get nervous about iterators being off by one)

I used to have each scope have an Option<LoopScope> embedded within, but I removed it at some point. Wonder if it'd be better that way. It was nice to be able to separate them, though.

Another option would be to allow a kind of marker to be pushed into the scopes array, so that as we iterate backwards we'd encounter a "track_break" marker and set the flag to true.

nagisa · 2016-01-19T20:02:05Z

@nikomatsakis nit-like comments addressed.

nikomatsakis · 2016-01-19T20:11:00Z

src/librustc_mir/build/expr/into.rs


                // start the loop
                this.cfg.terminate(block, Terminator::Goto { target: loop_block });

-                this.in_loop_scope(loop_block, exit_block, |this| {
+                let may_term = unpack!(exit_block = this.in_loop_scope(loop_block,
+                                                                       exit_block,


sorry to keep adding nits, but it seems like exit_block never changes, it just flows around and around. Maybe we should just change the closure's return type to (), and then we can make in_loop_scope return bool and drop this awkward unpack. The code will also be clearer in that it's clear that exit_block never changes (and I can't see why you would want it to?)

An attempt to make loop body destination be optional, author implemented a pretty self contained change and deemed it to be (much) uglier than the alternative of just keeping the unit temporary. Having the temporary created lazily also has a nice property of not figuring in the MIR of functions which do not use loops of any sort.

nikomatsakis · 2016-01-19T21:02:27Z

src/librustc_mir/build/expr/into.rs

                    if let Some(cond_expr) = opt_cond_expr {
+                        // This loop has a condition, ergo its exit_block is reachable.
+                        this.find_loop_scope(expr_span, None).might_break = true;


this seems fine; we could also check in the if below and say if may_break or opt_cond_expr.is_some()

To me it sounds like it is better to keep state consistent in case somebody else wants to use it later for whatever reasons.

nikomatsakis · 2016-01-19T21:12:46Z

This version looks good. I wonder if it's worth adding some tests (basically, the various cases we discussed), just to cement that this is the expected result of the compiler. Such tests may of course exist, I don't know. :(

e.g.:

http://is.gd/Fb92la should error
http://is.gd/fN01pG should not
http://is.gd/3mrB7r should not

(also, I know that these tests wouldn't test mir, at least not until we port borrowck etc to mir.)

nikomatsakis · 2016-01-19T21:13:25Z

well, r=me, I'll leave it to your discretion to consider whether adding tests seems to make sense, since I cannot decide :)

nagisa · 2016-01-19T21:38:39Z

@bors r=nikomatsakis 28638ed

bors · 2016-01-20T05:23:36Z

⌛ Testing commit 28638ed with merge 6271092...

bors · 2016-01-20T07:09:24Z

💔 Test failed - auto-linux-32-opt

alexcrichton · 2016-01-20T07:33:05Z

@bors: retry

On Tuesday, January 19, 2016, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-linux-32-opt
http://buildbot.rust-lang.org/builders/auto-linux-32-opt/builds/7778

—
Reply to this email directly or view it on GitHub
#30945 (comment).

Mostly testing borrowck and typeck behaviour in presence of `loop` expression

nagisa · 2016-01-20T11:28:56Z

@bors r=nikomatsakis 8877cca

bors · 2016-01-20T12:12:03Z

⌛ Testing commit 8877cca with merge d8c2360...

bors · 2016-01-20T14:19:42Z

💔 Test failed - auto-linux-64-nopt-t

nagisa · 2016-01-20T14:24:43Z

@bors retry

@nikomatsakis

As an attempt to make loop body destination be optional, author implemented a pretty self contained change and deemed it to be (much) uglier than the alternative of just keeping the unit temporary. Having the temporary created lazily also has a nice property of not figuring in the MIR of functions which do not use loops of any sort. r? @nikomatsakis

bors · 2016-01-20T22:03:34Z

⌛ Testing commit 8877cca with merge 4bb9d45...

bors · 2016-01-20T23:53:23Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

rust-highfive assigned nikomatsakis Jan 15, 2016

nagisa reviewed Jan 15, 2016
View reviewed changes

nagisa force-pushed the mir-optional-block-dest branch from 6a704a1 to 04895a0 Compare January 19, 2016 17:29

nikomatsakis reviewed Jan 19, 2016
View reviewed changes

nagisa force-pushed the mir-optional-block-dest branch from 04895a0 to 132b263 Compare January 19, 2016 19:58

nagisa reviewed Jan 19, 2016
View reviewed changes

nikomatsakis reviewed Jan 19, 2016
View reviewed changes

nagisa force-pushed the mir-optional-block-dest branch from 132b263 to 57b6ace Compare January 19, 2016 20:50

nagisa force-pushed the mir-optional-block-dest branch from 57b6ace to f9f6e3a Compare January 19, 2016 20:53

nikomatsakis reviewed Jan 19, 2016
View reviewed changes

Add tests for various diverging & converging loops

8877cca

Mostly testing borrowck and typeck behaviour in presence of `loop` expression

nagisa force-pushed the mir-optional-block-dest branch from 28638ed to 8877cca Compare January 20, 2016 11:28

bors merged commit 8877cca into rust-lang:master Jan 20, 2016

nagisa mentioned this pull request Jan 22, 2016

[MIR] Loops ignore their destination lvalue #30637

Closed

[MIR] Reintroduce the unit temporary #30945

[MIR] Reintroduce the unit temporary #30945

Uh oh!

Conversation

nagisa commented Jan 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nagisa commented Jan 15, 2016

Uh oh!

nagisa commented Jan 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Jan 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nagisa commented Jan 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Jan 19, 2016

Uh oh!

nikomatsakis commented Jan 19, 2016

Uh oh!

nagisa commented Jan 19, 2016

Uh oh!

bors commented Jan 20, 2016

Uh oh!

bors commented Jan 20, 2016

Uh oh!

alexcrichton commented Jan 20, 2016

Uh oh!

nagisa commented Jan 20, 2016

Uh oh!

bors commented Jan 20, 2016

Uh oh!

bors commented Jan 20, 2016

Uh oh!

nagisa commented Jan 20, 2016

Uh oh!

bors commented Jan 20, 2016

Uh oh!

bors commented Jan 20, 2016

Uh oh!

Uh oh!