[MIR] SwitchInt Everywhere #39456

nagisa · 2017-02-02T07:10:12Z

Something I've been meaning to do for a very long while. This PR essentially gets rid of 3 kinds of conditional branching and only keeps the most general one - SwitchInt. Primary benefits are such that dealing with MIR now does not involve dealing with 3 different ways to do conditional control flow. On the other hand, constructing a SwitchInt currently requires more code than what previously was necessary to build an equivalent If terminator. Something trivially "fixable" with some constructor methods somewhere (MIR needs stuff like that badly in general).

Some timings (tl;dr: slightly faster^1 (unexpected), but also uses slightly more memory at peak (expected)):

^1: Not sure if the speed benefits are because of LLVM liking the generated code better or the compiler itself getting compiled better. Either way, its a net benefit. The CORE and SYNTAX timings done for compilation without optimisation.

AFTER:
Building stage1 std artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 31.50 secs
    Finished release [optimized] target(s) in 31.42 secs
Building stage1 compiler artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 439.56 secs
    Finished release [optimized] target(s) in 435.15 secs

CORE: 99% (24.81 real, 0.13 kernel, 24.57 user); 358536k resident
CORE: 99% (24.56 real, 0.15 kernel, 24.36 user); 359168k resident
SYNTAX: 99% (49.98 real, 0.48 kernel, 49.42 user); 653416k resident
SYNTAX: 99% (50.07 real, 0.58 kernel, 49.43 user); 653604k resident

BEFORE:
Building stage1 std artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 31.84 secs
Building stage1 compiler artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 451.17 secs

CORE: 99% (24.66 real, 0.20 kernel, 24.38 user); 351096k resident
CORE: 99% (24.36 real, 0.17 kernel, 24.18 user); 352284k resident
SYNTAX: 99% (52.24 real, 0.56 kernel, 51.66 user); 645544k resident
SYNTAX: 99% (51.55 real, 0.48 kernel, 50.99 user); 646428k resident

cc @nikomatsakis @eddyb

rust-highfive · 2017-02-02T07:10:27Z

r? @nikomatsakis

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

nikomatsakis · 2017-02-02T14:25:27Z

So my take on this has traditionally been that I would prefer to do it as a kind of LOWERING step. That is, we would retain the semantic information when we build the tree, but lose it after dropck. However, I am not sure that this has a lot of value, and I'd be willing to go forward with this PR (after I give it a bit more of a look...), with the understanding that if, at some time later, we want to reason about the fact that some particular integer represents an enum discriminant, we may modify MIR to make that more direct (of course, one could also imagine tracking that fact via some kind of limited abstract interpretation or other analysis).

nikomatsakis

Overall this feels pretty nice. I am definitely +1 on merging If and SwitchInt (and have been for a while). As I wrote earlier, merging Switch and SwitchInt also seems ok to me for now at least. =)

nikomatsakis · 2017-02-02T14:29:41Z

src/librustc/mir/tcx.rs

@@ -195,7 +209,7 @@ impl<'tcx> Rvalue<'tcx> {
                    }
                }
            }
-            &Rvalue::InlineAsm { .. } => None
+            Rvalue::InlineAsm { .. } => None


Huh, this is pre-existing and not a problem with this PR per se, but I think that every rvalue should have a type. The cases where we see None here are, I think, basically bugs. For example, inline-asm should probably be a statement, if it has no result, and AggregateKind::Array should store the type as part of its variant, to handle the empty case. Is there a strong reason for it not to be this way?

Is there a strong reason for it not to be this way?

IIRC it has been this way all the way back to when first MIR patches landed. I have no idea why is it this way at the moment and I do not see any reason why it should stay an Rvalue.

OK, seems fine (but independent from this PR)

nikomatsakis · 2017-02-02T14:30:52Z

src/librustc/mir/tcx.rs

+                    };
+                    Some(ty)
+                } else {
+                    None


I think we should just always return int here. But I am biased (see below) against this function even returning an Option<> to begin with.

This really depends on the semantics we pick for the Discriminant rvalue. If we want it to be UB for non-Adt, it seems fine to me to return int. Otherwise I’d just report a bug! here.

nikomatsakis · 2017-02-02T14:33:58Z

src/librustc_trans/mir/rvalue.rs

+                let enum_ty = discr_lvalue.ty.to_ty(bcx.tcx());
+                let discr_ty = rvalue.ty(&*self.mir, bcx.tcx()).unwrap();
+                let discr_type = type_of::immediate_type_of(bcx.ccx, discr_ty);
+                // FIXME: inline this


Why inline this? Is it the only place it is called? (Even so, why inline it?)

This is what I’d expect would happen to this function once we finally remove the old trans. Since now MIR can express the Discriminant rvalue, it is possible to port all the old trans bits which deal with discriminants (e.g. discriminant intrinsic) to the MIR trans.

That being said, this FIXME should not exist currently, as there a fair amount of work involved in making this happen still.

nikomatsakis · 2017-02-02T14:37:25Z

src/librustc_typeck/collect.rs

+    let hint = UncheckedExprHint(ty_hint);
+    match ConstContext::new(ccx.tcx, body).eval(e, hint) {
+        Ok(ConstVal::Integral(i)) => {
+            // FIXME: eval should return an error if the hint is wrong


What is this FIXME talking about? I don't follow.

It is a preexisting FIXME, it is saying that const-evaluation should fail if hint is incorrect so the check below shouldn’t be necessary. I should double-check whether it is not fixed yet.

nikomatsakis · 2017-02-02T14:38:04Z

src/librustc/ty/layout.rs

-    fn repr_discr(tcx: TyCtxt, ty: Ty, hints: &[attr::ReprAttr], min: i64, max: i64)
-                      -> (Integer, bool) {
+    pub fn repr_discr(tcx: TyCtxt, hints: &[attr::ReprAttr], min: i128, max: i128)
+    -> (Integer, bool) {


Nit: formatting is unusual.

nikomatsakis · 2017-02-02T14:42:55Z

src/librustc/mir/mod.rs

@@ -471,6 +464,7 @@ pub enum TerminatorKind<'tcx> {

        /// Possible values. The locations to branch to in each case
        /// are found in the corresponding indices from the `targets` vector.
+        // FIXME: ConstVal doesn’t quite make any sense here? Its a Switch*Int*.
        values: Vec<ConstVal>,

        /// Possible branch sites. The length of this vector should be


It seems like we may want to adjust SwitchInt here -- perhaps you did so but I didn't notice, but it so this comment is out of date. The current comment suggests you always need a "fall-through" case, but we don't want that now, right? Although I guess that we could code it up by loading the set of discriminants (let's call them D[i] for i = 1...n) and then generating branches for D[i] where i in 1...n-1, and using the fall through case for D[n]. That is probably better, actually, as I'd prefer to keep this uniform.

This comment is not out of date. That being said I think it could make sense to adjust SwitchInt to be something like this instead:

values: Vec<(ConstVal, BasicBlock)>, otherwise: Option<BasicBlock> // if None, is exhaustive Switch.

At the very least setup as above would make invariants obviously clear, rather than fuzzy like it is now. Does that sound sensible?

Current setup is nice as the last block always gets considered an otherwise (without much effort, as you describe), but is also easy to overlook.

nikomatsakis · 2017-02-02T14:43:46Z

src/librustc_borrowck/borrowck/mir/elaborate_drops.rs

+                        kind: TerminatorKind::SwitchInt {
+                            discr: Operand::Consume(discr),
+                            switch_ty: discr_ty,
+                            values: values,


If we were to adopt the strategy I advocated above, then values has one too many elements. In any case, this seems to be wrong today, unless the comment is out of date.

nikomatsakis · 2017-02-02T14:44:11Z

src/librustc_mir/build/matches/test.rs

+                }
+                debug!("num_enum_variants: {}, tested variants: {:?}, variants: {:?}",
+                       num_enum_variants, values, variants);
+                // FIXME: WHY THIS DOES NOT WORK?!


Again, how does it fail?

This fails with an error that looks like this:

error[E0495]: cannot infer an appropriate lifetime for lifetime parameter 'tcx in function call due to conflicting requirements --> src/librustc/mir/tcx.rs:177:43 | 177 | Some(adt_def.discr_ty.to_ty(tcx)) | ^^^^^

And I have no idea where the conflict arises – everything looks correct to me. The function being called looks like this:

impl IntTypeExt for attr::IntType { fn to_ty<'a, 'gcx, 'tcx>(self, tcx: TyCtxt<'a, 'gcx, 'tcx>) -> Ty<'tcx> { match self { SignedInt(i) => tcx.mk_mach_int(i), UnsignedInt(i) => tcx.mk_mach_uint(i), } } }

Do you have an idea as to why it happens?

Figured it out. Bounds on to_ty are too relaxed.

nikomatsakis · 2017-02-02T14:46:32Z

src/librustc_mir/build/expr/into.rs

@@ -120,15 +119,15 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                this.cfg.terminate(block, source_info, TerminatorKind::SwitchInt {
                    discr: lhs,
                    switch_ty: this.hir.bool_ty(),
-                    values: vec![ConstVal::Bool(true)],
+                    values: BOOL_SWITCH_TRUE.clone(),


Nit: I think I'd expect, when using switch, that we set it up as false => ..., _ => ..., since in C one normally interprets "non-zero == true". Maybe it leads to marginally more efficiency assembly.

Acknowledged.

nikomatsakis · 2017-02-02T14:48:32Z

src/librustc_mir/transform/simplify_branches.rs

+                    literal: Literal::Value { ref value }, ..
+                }), ref values, ref targets, .. } => {
+                    if let Some(ref constint) = value.to_const_int() {
+                        let (otherwise, targets) = targets.split_last().unwrap();


This code suggests the "blocks 1 longer than values invariant" is still in effect.

arielb1 · 2017-02-02T22:24:00Z

Could you add a TerminatorKind::if constructor?

eddyb · 2017-02-03T00:52:24Z

src/librustc_typeck/collect.rs

+/// Find the appropriate type for the given signed discriminant range and #[repr] attribute.
+/// N.B.: u128 values above i128::MAX will be treated as signed, but that shouldn't affect
+/// anything, other than maybe debuginfo.
+pub fn repr_discr(tcx: TyCtxt, hints: &[attr::ReprAttr], min: i128, max: i128) -> attr::IntType {


Please don't use the results of evaluating constants in collect.

bors · 2017-02-04T01:33:14Z

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

nagisa · 2017-02-04T03:23:55Z

I feel like this is pretty much done ± the fixes for test failures. If the travis passes, its ready to be r+d.

arielb1 · 2017-02-04T19:44:14Z

src/librustc_borrowck/borrowck/mir/elaborate_drops.rs

@@ -644,13 +645,13 @@ impl<'b, 'tcx> ElaborateDropsCtxt<'b, 'tcx> {
                variant_path,
                &adt.variants[variant_index],
                substs);
-            self.drop_ladder(c, fields)
+            (self.drop_ladder(c, fields), true)
        } else {


move this logic to open_drop_for_adt instead of messing with booleans?

arielb1 · 2017-02-04T19:50:15Z

src/librustc_mir/build/matches/test.rs

-                             targets: targets
-                         })
-                    }
+                let (values, targets, ret) = if switch_ty.sty == ty::TyBool {


Use the old code with TerminatorKind::if_?

bors · 2017-02-05T16:58:28Z

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

nikomatsakis · 2017-02-07T04:32:38Z

src/librustc/mir/mod.rs

+        // otherwise: Option<BasicBlock> // exhaustive if None
+        //
+        // However we’ve decided to keep this as-is until we figure a case
+        // where some other approach seems to be strictly better than other.


I think the reason that I didn't do it this way is so that one could easily get a [BasicBlock] of places you might branch to. We could also just have the type-checker or other sanity checkers assert this property dynamically, which would help.

nikomatsakis · 2017-02-07T04:34:38Z

@bors r+

bors · 2017-02-07T04:34:39Z

📌 Commit dfea053 has been approved by nikomatsakis

nikomatsakis · 2017-02-07T04:35:06Z

@bors r-

nikomatsakis · 2017-02-07T04:35:48Z

So, I'm happy with this PR, but I want to give @rust-lang/compiler a chance to weigh in. I'll r+ a litle later today, unless anyone raises any blocking objections in the next few hours. =)

nikomatsakis · 2017-02-07T15:21:30Z

@bors r+

But @eddyb and @pnkfelix have a side-condition: @eddyb stipulates that you must change the name to Switch. And @pnkfelix that you can't do it now so as not to disturb "in-flight" PRs. So you have to come back and do it later. :)

bors · 2017-02-07T15:21:31Z

📌 Commit dfea053 has been approved by nikomatsakis

nagisa · 2017-02-10T18:23:49Z

Should be okay for 'nother go at bots.

Because certain somebody sucks at resolving big conflicts

eddyb · 2017-02-12T09:06:27Z

@bors r=nikomatsakis

bors · 2017-02-12T09:06:28Z

📌 Commit b663d9d has been approved by nikomatsakis

alexcrichton · 2017-02-13T02:28:07Z

@bors: retry

alexcrichton · 2017-02-13T02:28:54Z

@bors: r=nikomatsakis

bors · 2017-02-13T02:28:55Z

💡 This pull request was already approved, no need to approve it again.

bors · 2017-02-13T02:28:55Z

📌 Commit b663d9d has been approved by nikomatsakis

alexcrichton · 2017-02-13T02:32:05Z

@bors: retry

bors · 2017-02-13T02:32:13Z

⌛ Testing commit b663d9d with merge 05a7f25...

@nikomatsakis

[MIR] SwitchInt Everywhere Something I've been meaning to do for a very long while. This PR essentially gets rid of 3 kinds of conditional branching and only keeps the most general one - `SwitchInt`. Primary benefits are such that dealing with MIR now does not involve dealing with 3 different ways to do conditional control flow. On the other hand, constructing a `SwitchInt` currently requires more code than what previously was necessary to build an equivalent `If` terminator. Something trivially "fixable" with some constructor methods somewhere (MIR needs stuff like that badly in general). Some timings (tl;dr: slightly faster^1 (unexpected), but also uses slightly more memory at peak (expected)): ^1: Not sure if the speed benefits are because of LLVM liking the generated code better or the compiler itself getting compiled better. Either way, its a net benefit. The CORE and SYNTAX timings done for compilation without optimisation. ``` AFTER: Building stage1 std artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu) Finished release [optimized] target(s) in 31.50 secs Finished release [optimized] target(s) in 31.42 secs Building stage1 compiler artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu) Finished release [optimized] target(s) in 439.56 secs Finished release [optimized] target(s) in 435.15 secs CORE: 99% (24.81 real, 0.13 kernel, 24.57 user); 358536k resident CORE: 99% (24.56 real, 0.15 kernel, 24.36 user); 359168k resident SYNTAX: 99% (49.98 real, 0.48 kernel, 49.42 user); 653416k resident SYNTAX: 99% (50.07 real, 0.58 kernel, 49.43 user); 653604k resident BEFORE: Building stage1 std artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu) Finished release [optimized] target(s) in 31.84 secs Building stage1 compiler artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu) Finished release [optimized] target(s) in 451.17 secs CORE: 99% (24.66 real, 0.20 kernel, 24.38 user); 351096k resident CORE: 99% (24.36 real, 0.17 kernel, 24.18 user); 352284k resident SYNTAX: 99% (52.24 real, 0.56 kernel, 51.66 user); 645544k resident SYNTAX: 99% (51.55 real, 0.48 kernel, 50.99 user); 646428k resident ``` cc @nikomatsakis @eddyb

bors · 2017-02-13T04:57:53Z

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

If I’m reading the release-mode assembly correctly, before this change PropertyDeclaration::id is implemented with a computed jump: ```assembly lea rcx, [rip + .LJTI117_0] movsxd rax, dword ptr [rcx + 4*rax] add rax, rcx jmp rax .LBB117_3: mov dword ptr [rdi], 65536 mov rax, rdi ret .LBB117_2: mov dword ptr [rdi], 0 mov rax, rdi ret .LBB117_4: mov dword ptr [rdi], 131072 mov rax, rdi ret .LBB117_6: mov dword ptr [rdi], 262144 mov rax, rdi ret .LBB117_7: mov dword ptr [rdi], 327680 mov rax, rdi ret ; Four similar lines repeated for each of the few hundred variants... ``` With Rust 1.15 (currently used for geckolib) this doesn’t change significantly. In Nightly 1.17 however, the compiled code uses a lookup table, possibly thanks to rust-lang/rust#39456. ```assembly movq (%rsi), %rax cmpq $171, %rax jne .LBB23_1 addq $8, %rsi movq %rsi, 8(%rdi) movb $1, %al jmp .LBB23_3 .LBB23_1: xorq $128, %rax leaq .Lswitch.table.6(%rip), %rcx movb (%rax,%rcx), %al movb %al, 1(%rdi) xorl %eax, %eax .LBB23_3: movb %al, (%rdi) movq %rdi, %rax retq ```

Rewrite PropertyDeclaration::id to help the optimizer. If I’m reading the release-mode assembly correctly, before this change `PropertyDeclaration::id` is implemented with a computed jump: ```assembly lea rcx, [rip + .LJTI117_0] movsxd rax, dword ptr [rcx + 4*rax] add rax, rcx jmp rax .LBB117_3: mov dword ptr [rdi], 65536 mov rax, rdi ret .LBB117_2: mov dword ptr [rdi], 0 mov rax, rdi ret .LBB117_4: mov dword ptr [rdi], 131072 mov rax, rdi ret .LBB117_6: mov dword ptr [rdi], 262144 mov rax, rdi ret .LBB117_7: mov dword ptr [rdi], 327680 mov rax, rdi ret ; Four similar lines repeated for each of the few hundred variants... ``` With Rust 1.15 (currently used for geckolib) this doesn’t change significantly. In Nightly 1.17 however, the compiled code uses a lookup table, possibly thanks to rust-lang/rust#39456. ```assembly movq (%rsi), %rax cmpq $171, %rax jne .LBB23_1 addq $8, %rsi movq %rsi, 8(%rdi) movb $1, %al jmp .LBB23_3 .LBB23_1: xorq $128, %rax leaq .Lswitch.table.6(%rip), %rcx movb (%rax,%rcx), %al movb %al, 1(%rdi) xorl %eax, %eax .LBB23_3: movb %al, (%rdi) movq %rdi, %rax retq ```  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15992)

…imizer (from servo:id-table); r=bholley If I’m reading the release-mode assembly correctly, before this change `PropertyDeclaration::id` is implemented with a computed jump: ```assembly lea rcx, [rip + .LJTI117_0] movsxd rax, dword ptr [rcx + 4*rax] add rax, rcx jmp rax .LBB117_3: mov dword ptr [rdi], 65536 mov rax, rdi ret .LBB117_2: mov dword ptr [rdi], 0 mov rax, rdi ret .LBB117_4: mov dword ptr [rdi], 131072 mov rax, rdi ret .LBB117_6: mov dword ptr [rdi], 262144 mov rax, rdi ret .LBB117_7: mov dword ptr [rdi], 327680 mov rax, rdi ret ; Four similar lines repeated for each of the few hundred variants... ``` With Rust 1.15 (currently used for geckolib) this doesn’t change significantly. In Nightly 1.17 however, the compiled code uses a lookup table, possibly thanks to rust-lang/rust#39456. ```assembly movq (%rsi), %rax cmpq $171, %rax jne .LBB23_1 addq $8, %rsi movq %rsi, 8(%rdi) movb $1, %al jmp .LBB23_3 .LBB23_1: xorq $128, %rax leaq .Lswitch.table.6(%rip), %rcx movb (%rax,%rcx), %al movb %al, 1(%rdi) xorl %eax, %eax .LBB23_3: movb %al, (%rdi) movq %rdi, %rax retq ``` Source-Repo: https://github.com/servo/servo Source-Revision: 9e8e1a47241c6906b4f5777da0d04e3655982fae --HG-- extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : 80fb6158601e5f4715805013dd7c9492ceb38461

…imizer (from servo:id-table); r=bholley If I’m reading the release-mode assembly correctly, before this change `PropertyDeclaration::id` is implemented with a computed jump: ```assembly lea rcx, [rip + .LJTI117_0] movsxd rax, dword ptr [rcx + 4*rax] add rax, rcx jmp rax .LBB117_3: mov dword ptr [rdi], 65536 mov rax, rdi ret .LBB117_2: mov dword ptr [rdi], 0 mov rax, rdi ret .LBB117_4: mov dword ptr [rdi], 131072 mov rax, rdi ret .LBB117_6: mov dword ptr [rdi], 262144 mov rax, rdi ret .LBB117_7: mov dword ptr [rdi], 327680 mov rax, rdi ret ; Four similar lines repeated for each of the few hundred variants... ``` With Rust 1.15 (currently used for geckolib) this doesn’t change significantly. In Nightly 1.17 however, the compiled code uses a lookup table, possibly thanks to rust-lang/rust#39456. ```assembly movq (%rsi), %rax cmpq $171, %rax jne .LBB23_1 addq $8, %rsi movq %rsi, 8(%rdi) movb $1, %al jmp .LBB23_3 .LBB23_1: xorq $128, %rax leaq .Lswitch.table.6(%rip), %rcx movb (%rax,%rcx), %al movb %al, 1(%rdi) xorl %eax, %eax .LBB23_3: movb %al, (%rdi) movq %rdi, %rax retq ``` Source-Repo: https://github.com/servo/servo Source-Revision: 9e8e1a47241c6906b4f5777da0d04e3655982fae

If I’m reading the release-mode assembly correctly, before this change PropertyDeclaration::id is implemented with a computed jump: ```assembly lea rcx, [rip + .LJTI117_0] movsxd rax, dword ptr [rcx + 4*rax] add rax, rcx jmp rax .LBB117_3: mov dword ptr [rdi], 65536 mov rax, rdi ret .LBB117_2: mov dword ptr [rdi], 0 mov rax, rdi ret .LBB117_4: mov dword ptr [rdi], 131072 mov rax, rdi ret .LBB117_6: mov dword ptr [rdi], 262144 mov rax, rdi ret .LBB117_7: mov dword ptr [rdi], 327680 mov rax, rdi ret ; Four similar lines repeated for each of the few hundred variants... ``` With Rust 1.15 (currently used for geckolib) this doesn’t change significantly. In Nightly 1.17 however, the compiled code uses a lookup table, possibly thanks to rust-lang/rust#39456. ```assembly movq (%rsi), %rax cmpq $171, %rax jne .LBB23_1 addq $8, %rsi movq %rsi, 8(%rdi) movb $1, %al jmp .LBB23_3 .LBB23_1: xorq $128, %rax leaq .Lswitch.table.6(%rip), %rcx movb (%rax,%rcx), %al movb %al, 1(%rdi) xorl %eax, %eax .LBB23_3: movb %al, (%rdi) movq %rdi, %rax retq ```

…imizer (from servo:id-table); r=bholley If I’m reading the release-mode assembly correctly, before this change `PropertyDeclaration::id` is implemented with a computed jump: ```assembly lea rcx, [rip + .LJTI117_0] movsxd rax, dword ptr [rcx + 4*rax] add rax, rcx jmp rax .LBB117_3: mov dword ptr [rdi], 65536 mov rax, rdi ret .LBB117_2: mov dword ptr [rdi], 0 mov rax, rdi ret .LBB117_4: mov dword ptr [rdi], 131072 mov rax, rdi ret .LBB117_6: mov dword ptr [rdi], 262144 mov rax, rdi ret .LBB117_7: mov dword ptr [rdi], 327680 mov rax, rdi ret ; Four similar lines repeated for each of the few hundred variants... ``` With Rust 1.15 (currently used for geckolib) this doesn’t change significantly. In Nightly 1.17 however, the compiled code uses a lookup table, possibly thanks to rust-lang/rust#39456. ```assembly movq (%rsi), %rax cmpq $171, %rax jne .LBB23_1 addq $8, %rsi movq %rsi, 8(%rdi) movb $1, %al jmp .LBB23_3 .LBB23_1: xorq $128, %rax leaq .Lswitch.table.6(%rip), %rcx movb (%rax,%rcx), %al movb %al, 1(%rdi) xorl %eax, %eax .LBB23_3: movb %al, (%rdi) movq %rdi, %rax retq ``` Source-Repo: https://github.com/servo/servo Source-Revision: 9e8e1a47241c6906b4f5777da0d04e3655982fae UltraBlame original commit: af90f70b05df8c81f440b222728ae48c2e2bf5f8

rust-highfive assigned nikomatsakis Feb 2, 2017

nikomatsakis requested changes Feb 2, 2017

View reviewed changes

nagisa force-pushed the mir-switchint-everywhere branch from fd2cc21 to f2147bd Compare February 2, 2017 19:17

eddyb reviewed Feb 3, 2017

View reviewed changes

nagisa force-pushed the mir-switchint-everywhere branch 3 times, most recently from 1f945cb to 90b5c70 Compare February 3, 2017 22:28

nagisa force-pushed the mir-switchint-everywhere branch 3 times, most recently from f756897 to 1eff9d3 Compare February 4, 2017 03:07

nagisa force-pushed the mir-switchint-everywhere branch 2 times, most recently from 79b29b0 to 2087848 Compare February 4, 2017 09:54

arielb1 reviewed Feb 4, 2017

View reviewed changes

nagisa force-pushed the mir-switchint-everywhere branch 2 times, most recently from 0b02d79 to dfea053 Compare February 6, 2017 01:12

nikomatsakis reviewed Feb 7, 2017

View reviewed changes

nikomatsakis approved these changes Feb 7, 2017

View reviewed changes

nagisa force-pushed the mir-switchint-everywhere branch from 7964f16 to 075ae04 Compare February 10, 2017 18:20

Rebase fallout

b663d9d

Because certain somebody sucks at resolving big conflicts

nagisa force-pushed the mir-switchint-everywhere branch from 075ae04 to b663d9d Compare February 11, 2017 06:10

bors merged commit b663d9d into rust-lang:master Feb 13, 2017

nox mentioned this pull request Mar 13, 2017

Some perf tweaks to the tokenizer. servo/rust-cssparser#112

Merged

SimonSapin mentioned this pull request Mar 13, 2017

Reconsider match_byte macro if plain match becomes optimized enough servo/rust-cssparser#128

Open

SimonSapin mentioned this pull request Mar 16, 2017

Rewrite PropertyDeclaration::id to help the optimizer. servo/servo#15992

Merged

[MIR] SwitchInt Everywhere #39456

[MIR] SwitchInt Everywhere #39456

Conversation

nagisa commented Feb 2, 2017 • edited by alexcrichton Loading

rust-highfive commented Feb 2, 2017

nikomatsakis commented Feb 2, 2017

nikomatsakis left a 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

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

arielb1 commented Feb 2, 2017

Choose a reason for hiding this comment

bors commented Feb 4, 2017

nagisa commented Feb 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Feb 5, 2017

Choose a reason for hiding this comment

nikomatsakis commented Feb 7, 2017

bors commented Feb 7, 2017

nikomatsakis commented Feb 7, 2017

nikomatsakis commented Feb 7, 2017

nikomatsakis commented Feb 7, 2017

bors commented Feb 7, 2017

nagisa commented Feb 10, 2017

eddyb commented Feb 12, 2017 • edited by alexcrichton Loading

bors commented Feb 12, 2017

alexcrichton commented Feb 13, 2017

alexcrichton commented Feb 13, 2017

bors commented Feb 13, 2017

bors commented Feb 13, 2017

alexcrichton commented Feb 13, 2017

bors commented Feb 13, 2017

bors commented Feb 13, 2017

nagisa commented Feb 2, 2017 •

edited by alexcrichton

Loading

eddyb commented Feb 12, 2017 •

edited by alexcrichton

Loading