turn on MIR construction unconditionally #28748

nikomatsakis · 2015-09-30T09:14:26Z

I had to fix a few things. Notable changes:

I removed the MIR support for constants, instead falling back to the existing ConstVal. I still think we ought to reform how we handle constants, but it's not clear to me that the approach I was taking is correct, and anyway I think we ought to do it separately.
I adjusted how we handle bindings in matches: we now declare all the bindings up front, rather than doing it as we encounter them. This is not only simpler, since we don't have to check if a binding has already been declared, it avoids ICEs if any of the arms turn out to be unreachable.
I do MIR construction after check_match, because it detects various broken cases. I'd like for check_match to be subsumed by MIR construction, but we can do that as a separate PR (if indeed it makes sense).

I did a crater run and found no regressions in the wild: https://gist.github.com/nikomatsakis/0038f90e10c8ad00f2f8

rust-highfive · 2015-09-30T09:14:39Z

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

nikomatsakis · 2015-09-30T11:26:41Z

Oh, I meant to remove the "reachable" flags for matches. I can do that easily enough.

nikomatsakis · 2015-09-30T12:32:30Z

done.

eefriedman · 2015-09-30T18:20:59Z

src/librustc/middle/const_eval.rs

+    #[stable(feature = "rust1", since = "1.0.0")]
+    fn eq(&self, other: &ConstVal) -> bool {
+        match (self, other) {
+            (&Float(v1), &Float(v2)) => (v1.is_nan() && v2.is_nan()) || (v1 == v2),


Is v1.is_nan() && v2.is_nan() really what you mean, as opposed to transmute<f64, u64>(v1) == transmute<f64, u64>(v2)?

nikomatsakis · 2015-09-30T18:29:22Z

@eefriedman

Is v1.is_nan() && v2.is_nan() really what you mean, as opposed to transmute<f64, u64>(v1) == transmute<f64, u64>(v2)?

Hmm, interesting question. It's probably better to be more conservative, I agree.

alexcrichton · 2015-09-30T18:58:17Z

🎊

nikomatsakis · 2015-09-30T19:02:32Z

@alexcrichton huh, that confetti-ball renders looks more like an angry stormcloud to me. I was like "what did I do to make alex angry now?"

alexcrichton · 2015-09-30T19:08:40Z

😈

pnkfelix · 2015-10-01T13:46:27Z

Hmm, on a checkout from yesterday, the -Z always-build-mir is regressing rustc on this test case: playpen

#[derive(Copy, Clone, PartialEq, Eq, Debug)]
#[repr(usize)]
pub enum E { A = 0, B = 1 }

impl E {
    pub fn foo(x: usize) -> Self {
        use self::E::{A, B};
        const E_A: usize = A as usize;
        const E_B: usize = B as usize;
        match x {
            E_A => A,
            E_B => B,
            _ => panic!("no match"),
        }
    }
}

results in ICE:

/tmp/foo.rs:8:28: 8:38 error: internal compiler error: expression is not a valid constant Cast { source: Hair(expr(262: A)) }
/tmp/foo.rs:8         const E_A: usize = A as usize;
                                         ^~~~~~~~~~

@nikomatsakis

The crater run you did, I assume it wasn't including the -Z always-build-mir on the "before" step, right? I'm just a little surprised that you did not see examples of this pattern in use...

pnkfelix · 2015-10-01T13:49:12Z

(The reason that I point this out on this ticket is just to warn that we do not seem to have been testing this pattern internally, if -Z always-build-mir is always being passed ... but maybe its not being passed during the test suite runs? In any case, I have not confirmed that the above ICE persists after applying this PR; for all I know it might well fix the above noted bug.)

Update: and in fact the commit messages for the commit series on this PR gives me some hope that this problem is in fact addressed somewhere here, since this PR does something with respect to the handling of patterns and const-evaluation.

Update 2: @nikomatsakis I will actually build rust with this PR locally and check whether it indeed does fix the test case I listed above.

nikomatsakis · 2015-10-01T15:26:43Z

@pnkfelix that test case works on this branch. I did indeed revamp how constants are handled to just reuse more of the compiler's existing machinery.

nikomatsakis · 2015-10-01T17:47:26Z

Note: -Z always build Mir is only used currently when bootstrapping, not
during test runs.
On Oct 1, 2015 9:49 AM, "Felix S Klock II" notifications@github.com wrote:

(The reason that I point this out on this ticket is just to warn that we
do not seem to have been testing this pattern internally, if -Z
always-build-mir is always being passed ... but maybe its not being
passed during the test suite runs? In any case, I have not confirmed that
the above ICE persists after applying this PR; for all I know it might well
fix the above noted bug.)

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

bors · 2015-10-01T18:17:22Z

☔ The latest upstream changes (presumably #28742) made this pull request unmergeable. Please resolve the merge conflicts.

pnkfelix · 2015-10-02T13:43:29Z

@nikomatsakis okay I've finished reviewing. Looks fine, assuming you squash some of the later commits into the appropriate earlier commits (i.e. the removal of the reaches array and the change to NaN handling commits should both get squashed into earlier commits).

So r=me once that's done.

various annoying edge cases

trees in MIR

and track which arms are reached (though in fact we don't make use of this right now -- we might later if we absorb the checking of patterns into MIR, as I would like)

cause panics

if they represent the same constant; otherwise the match algorithm goes into infinite recursion when a pattern contains `NaN`

nikomatsakis · 2015-10-05T01:21:22Z

@bors r=pnkfelix

bors · 2015-10-05T01:21:22Z

📌 Commit 7e1e830 has been approved by pnkfelix

bors · 2015-10-05T03:06:34Z

⌛ Testing commit 7e1e830 with merge c298efd...

I had to fix a few things. Notable changes: 1. I removed the MIR support for constants, instead falling back to the existing `ConstVal`. I still think we ought to reform how we handle constants, but it's not clear to me that the approach I was taking is correct, and anyway I think we ought to do it separately. 2. I adjusted how we handle bindings in matches: we now *declare* all the bindings up front, rather than doing it as we encounter them. This is not only simpler, since we don't have to check if a binding has already been declared, it avoids ICEs if any of the arms turn out to be unreachable. 3. I do MIR construction *after* `check_match`, because it detects various broken cases. I'd like for `check_match` to be subsumed by MIR construction, but we can do that as a separate PR (if indeed it makes sense). I did a crater run and found no regressions in the wild: https://gist.github.com/nikomatsakis/0038f90e10c8ad00f2f8

bors · 2015-10-05T04:58:39Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, 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 pnkfelix Sep 30, 2015

nikomatsakis mentioned this pull request Sep 30, 2015

Tracking issue for MIR (RFC #1211) #27840

Closed

16 tasks

eefriedman reviewed Sep 30, 2015
View reviewed changes

nikomatsakis force-pushed the universal-mir branch from 9adce8d to c84f284 Compare October 1, 2015 19:08

nikomatsakis added 3 commits October 4, 2015 21:08

always build MIR

0d207cb

do MIR construction after pattern evaluation for now to sidestep

dedde0b

various annoying edge cases

switch to using constvals for constants, instead of having constant

1f4acfa

trees in MIR

nikomatsakis force-pushed the universal-mir branch from c84f284 to 5261441 Compare October 5, 2015 01:13

nikomatsakis added 3 commits October 4, 2015 21:19

rather than just passing the block around, track the candidate's index,

96a3cd0

and track which arms are reached (though in fact we don't make use of this right now -- we might later if we absorb the checking of patterns into MIR, as I would like)

change how we declare bindings so that unreachable arms don't

0197f98

cause panics

change PartialEq impl for ConstVal so that two constants are ==

7e1e830

if they represent the same constant; otherwise the match algorithm goes into infinite recursion when a pattern contains `NaN`

nikomatsakis force-pushed the universal-mir branch from 5261441 to 7e1e830 Compare October 5, 2015 01:21

bors merged commit 7e1e830 into rust-lang:master Oct 5, 2015

nikomatsakis deleted the universal-mir branch March 30, 2016 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

turn on MIR construction unconditionally #28748

turn on MIR construction unconditionally #28748

nikomatsakis commented Sep 30, 2015

rust-highfive commented Sep 30, 2015

nikomatsakis commented Sep 30, 2015

nikomatsakis commented Sep 30, 2015

eefriedman Sep 30, 2015

nikomatsakis commented Sep 30, 2015

alexcrichton commented Sep 30, 2015

nikomatsakis commented Sep 30, 2015

alexcrichton commented Sep 30, 2015

pnkfelix commented Oct 1, 2015

pnkfelix commented Oct 1, 2015

nikomatsakis commented Oct 1, 2015

nikomatsakis commented Oct 1, 2015

bors commented Oct 1, 2015

pnkfelix commented Oct 2, 2015

nikomatsakis commented Oct 5, 2015

bors commented Oct 5, 2015

bors commented Oct 5, 2015

bors commented Oct 5, 2015

turn on MIR construction unconditionally #28748

turn on MIR construction unconditionally #28748

Conversation

nikomatsakis commented Sep 30, 2015

rust-highfive commented Sep 30, 2015

nikomatsakis commented Sep 30, 2015

nikomatsakis commented Sep 30, 2015

eefriedman Sep 30, 2015

Choose a reason for hiding this comment

nikomatsakis commented Sep 30, 2015

alexcrichton commented Sep 30, 2015

nikomatsakis commented Sep 30, 2015

alexcrichton commented Sep 30, 2015

pnkfelix commented Oct 1, 2015

pnkfelix commented Oct 1, 2015

nikomatsakis commented Oct 1, 2015

nikomatsakis commented Oct 1, 2015

bors commented Oct 1, 2015

pnkfelix commented Oct 2, 2015

nikomatsakis commented Oct 5, 2015

bors commented Oct 5, 2015

bors commented Oct 5, 2015

bors commented Oct 5, 2015