always add an unreachable branch on matches to give more info to llvm #45821

djzin · 2017-11-07T07:27:02Z

As part of https://github.com/djzin/rustc-optimization I discovered that some simple enum optimizations (src/unary/three_valued_enum.rs and src/unary/four_valued_enum.rs in the repo) are not applied - and the reason for this is that we erase the info that the discriminant of an enum is one of the options by putting the last one in an "otherwise" branch. This patch adds an extra branch so that LLVM can know what the possibilities are for the discriminant, which fixes the three- and four- valued cases.

Note that for whatever reason, this doesn't fix the case of 2 variants (most notably Option and Result have 2 variants) - a pass re-ordering might fix this or we may wish to add "assume" annotations on discriminants to force it to optimize.

rust-highfive · 2017-11-07T07:27:15Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

nikomatsakis · 2017-11-07T15:35:28Z

cc @rust-lang/compiler -- Do we have any testing framework that could test this locally? I guess mir-opt wouldn't do it, we'd want to be checking the code that LLVM generates post-optimization.

kennytm · 2017-11-07T15:39:35Z

@nikomatsakis src/test/codegen?

The existing src/test/codegen/vec-optimizes-away.rs seem to be a test like this.

nikomatsakis · 2017-11-07T15:40:38Z

Yeah, so in Ye Olden Days we used to have a Switch that was targeted to enums, which handled this implicitly, but we removed it in favor of SwitchInt. I'm torn on whether this PR proposes the best fix -- it certainly aligns with what LLVM does but it feels a hair indirect. I wonder if it'd be nice to have a way on the SwitchInt to declare it to be exhaustive instead -- or perhaps to add "range" annotations to the temporary we use to store the result of the temporary?

OTOH, I see the appeal of LLVM's approach. If you have some existing branches that become undefined behavior for whatever reason, it all falls out naturally.

nikomatsakis · 2017-11-07T15:42:04Z

src/librustc_mir/build/matches/test.rs

@@ -205,7 +205,9 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                if let Some(otherwise_block) = otherwise_block {
                    targets.push(otherwise_block);
                } else {
-                    values.pop();
+                    let unreachable_block = self.cfg.start_new_block();


Presuming we do decide to keep this approach, this particular patch doesn't feel like the best:

It relies on the fact that start_new_block is returning a "unreachable" terminator by default. That feels like an implementation detail we may want to change.

It creates an extra block per match, when I suppose we only need one for the entire function, right?

I'd rather have something like self.cfg.unreachable_block() that lazilly (or eagerly...) creates such a block.

As I understand it, the blocks are not actually populated at all in this step. All of them are start_new_block. The code that then follows on from this goes through each case and fills them in. It already handles the "last case is unreachable" case because it already has to handle

let x: u8; match x { 0...255 => ..., _ => ... }

Please correct me if I'm wrong, I haven't worked with this code before :)

It creates an extra block per match, when I suppose we only need one for the entire function, right?

Not sure what you mean here? Surely we could be matching different things within the same function

So, I might have been remembering wrong in terms of how things work, I'll have to check -- but I don't think I am wrong that we only need one unreachable block per function. That is, why would we need more than one? All matches can share the same one, I should think.

Ah I see what you mean now - I will investigate how many blocks get generated for a function with many matches.

Hm have run into a snag with this. A Terminator has a TerminatorKind as well as a SourceInfo. Since SourceInfo necessarily differs between different matches, it is not possible to dedup the blocks, unless we put the wrong SourceInfo on some (unreachable) basic blocks.

So we do something similar for the cached_return_block. The idea is that everybody references this block, which has no terminator as of yet, and then we assign the terminator with the source info of the function end:

https://github.com/rust-lang/rust/blob/master/src/librustc_mir/build/mod.rs#L400-L401

We might do something like that here.

nikomatsakis · 2017-11-07T15:43:17Z

@kennytm yep thanks =) I suppose I could have switched over to my terminal to check myself; but I also wanted to solicit opinion on the other bits of this PR.

nikomatsakis · 2017-11-07T15:43:27Z

(Ooops, didn't mean to close.)

djzin · 2017-11-07T19:10:52Z

perhaps to add "range" annotations to the temporary we use to store the result of the temporary?

The problem with "range" annotations is that they can only be used to annotate loads / function calls for some reason and so they tend to disappear. For example;

define i8 @no_op(i8) {

    %temp = alloca i8
    store i8 %0, i8* %temp
    %value = load i8, i8* %temp, !range !{i8 0, i8 2}

    switch i8 %value, label %one [
        i8 0, label %zero
    ]

zero:
    br label %end

one:
    br label %end

end:
    %ret = phi i8 [0, %zero], [1, %one]
    ret i8 %ret
}

with opt -S -instcombine gives

define i8 @no_op(i8) {
  switch i8 %0, label %one [
    i8 0, label %zero
  ]

zero:                                             ; preds = %1
  br label %end

one:                                              ; preds = %1
  br label %end

end:                                              ; preds = %one, %zero
  %ret = phi i8 [ 0, %zero ], [ 1, %one ]
  ret i8 %ret
}

which leads to a lot of missed optimization opportunities.

In fact I think we already do annotate "range" in this way, but it gets dropped.

djzin · 2017-11-07T19:22:21Z

The other thing we can do is add a bunch of @llvm.assume all over the place - consider the output of

#[derive(Copy, Clone)]
pub enum Two { First, Second }
use Two::*;

#[no_mangle]
pub fn two_valued(x: Two) -> Two {
    match x {
        First => First,
        Second => Second,
    }
}

which is

define i8 @two_valued(i8) unnamed_addr #0 {
start:
  %not.cond = icmp ne i8 %0, 0
  %. = zext i1 %not.cond to i8
  ret i8 %.
}

two_valued:
	testb	%dil, %dil
	setne	%al
	retq

versus

#[no_mangle]
pub fn two_valued(x: Two) -> Two {
    unsafe {
        std::intrinsics::assume(2 > x as u8);
    }
    match x {
        First => First,
        Second => Second,
    }
}

which gives

define i8 @two_valued(i8) unnamed_addr #0 {
start:
  %1 = icmp ult i8 %0, 2
  tail call void @llvm.assume(i1 %1)
  ret i8 %0
}

two_valued:
	movl	%edi, %eax
	retq

nikomatsakis · 2017-11-08T17:19:43Z

@djzin well, so one question is how to tell LLVM that the match is exhaustive -- it seems like using an unreachable 'otherwise' block is fine there. The other question though is how to encode it in MIR. But I think I'm convinced we might as well use an unreachable block too. We can always change it after all.

nikomatsakis · 2017-11-10T17:18:38Z

@djzin (just in case it was not clear, I think we're settled on taking this route that you've proposed, but still waiting on adding some codegen tests that show it is working, along with minor tweaks to the body of the PR.)

djzin · 2017-11-13T23:25:58Z

made suggested changes

… about which values are possible

…ng brace

nikomatsakis · 2017-11-14T17:00:41Z

@djzin thanks!

nikomatsakis · 2017-11-14T17:00:47Z

@bors r+

bors · 2017-11-14T17:00:48Z

📌 Commit 5b1cc1d has been approved by nikomatsakis

always add an unreachable branch on matches to give more info to llvm As part of https://github.com/djzin/rustc-optimization I discovered that some simple enum optimizations (src/unary/three_valued_enum.rs and src/unary/four_valued_enum.rs in the repo) are not applied - and the reason for this is that we erase the info that the discriminant of an enum is one of the options by putting the last one in an "otherwise" branch. This patch adds an extra branch so that LLVM can know what the possibilities are for the discriminant, which fixes the three- and four- valued cases. Note that for whatever reason, this doesn't fix the case of 2 variants (most notably `Option` and `Result` have 2 variants) - a pass re-ordering might fix this or we may wish to add "assume" annotations on discriminants to force it to optimize.

bors · 2017-11-14T18:47:41Z

⌛ Testing commit 5b1cc1d with merge ff0f5de...

bors · 2017-11-14T21:20:40Z

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

eddyb · 2017-11-18T13:22:14Z

Something very weird is happening here... The MIR we build (AFAICT, by looking at the files produced by -Zdump-mir, with the .-------.mir_map.0.mir suffix), without any extra MIR optimizations, for the testcase added in this PR, is branchless.

Not only that but so far I've been unable to find where this actually happens in rustc_mir.

EDIT: oh wow the test is bogus because of the overlapping imports, there is no match.

eddyb · 2017-11-18T13:37:56Z

src/test/codegen/match-optimizes-away.rs

+use Three::*;
+
+pub enum Four { First, Second, Third, Fourth }
+use Four::*;


It'd be nice if we made a lint against this, set to error at least for the Rust codebase...

wow thanks for catching this, I had them in 2 separate files before & just assumed that any warning would trap here...

There are warnings from the matches themselves but most tests are written without warning avoidance in mind so we'd have to do a lot of work to ban warnings from tests.

Should probably as a matter of course put #![deny(warnings)] on these tests where it is appropriate (at least I will do that in future)

Fixes rust-lang#46843. rust-lang#45821 added unreachable blocks in matches, which were terminated in construct_fn but not in construct_const, causing a panic due to "no terminator on block" when constants involved matching on enums. The "unimplemented expression type" error may go away in the future, the key is that we see the E0015 about using a non-const function and then don't ICE.

MIR: terminate unreachable blocks in construct_const Fixes #46843. #45821 added unreachable blocks in matches, which were terminated in construct_fn but not in construct_const, causing a panic due to "no terminator on block" when constants involved matching on enums. The "unimplemented expression type" error may go away in the future, the key is that we see the E0015 about using a non-const function and then don't ICE.

rust-highfive assigned nikomatsakis Nov 7, 2017

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 7, 2017

nikomatsakis reviewed Nov 7, 2017

View reviewed changes

nikomatsakis closed this Nov 7, 2017

nikomatsakis reopened this Nov 7, 2017

djzin added 4 commits November 14, 2017 06:33

always add an unreachable branch on matches to give more info to llvm…

829b703

… about which values are possible

use lazy cached unreachable block - assign it to the function's closi…

aed0c9c

…ng brace

add optimization codegen tests

c503fe1

don't send block back to be marked unreachable twice

5b1cc1d

djzin force-pushed the unreachable-match-arms branch from 651af1a to 5b1cc1d Compare November 14, 2017 07:04

nikomatsakis approved these changes Nov 14, 2017

View reviewed changes

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 14, 2017

bors merged commit 5b1cc1d into rust-lang:master Nov 14, 2017

eddyb reviewed Nov 18, 2017

View reviewed changes

eddyb mentioned this pull request Nov 18, 2017

Don't glob-import overlapping variant names in test/codegen/match-optimizes-away.rs. #46078

Closed

kennytm mentioned this pull request Nov 19, 2017

Refactor type memory layouts and ABIs, to be more general and easier to optimize. #45225

Merged

This was referenced Dec 20, 2017

MIR: terminate unreachable blocks in construct_const #46877

Merged

ICE with match on non-const functions in constant definitions #46843

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

always add an unreachable branch on matches to give more info to llvm #45821

always add an unreachable branch on matches to give more info to llvm #45821

djzin commented Nov 7, 2017

rust-highfive commented Nov 7, 2017

nikomatsakis commented Nov 7, 2017

kennytm commented Nov 7, 2017 •

edited

Loading

nikomatsakis commented Nov 7, 2017

nikomatsakis Nov 7, 2017

djzin Nov 7, 2017 •

edited

Loading

djzin Nov 7, 2017

nikomatsakis Nov 8, 2017

djzin Nov 8, 2017 •

edited

Loading

djzin Nov 8, 2017

nikomatsakis Nov 9, 2017

nikomatsakis commented Nov 7, 2017

nikomatsakis commented Nov 7, 2017

djzin commented Nov 7, 2017 •

edited

Loading

djzin commented Nov 7, 2017 •

edited

Loading

nikomatsakis commented Nov 8, 2017

nikomatsakis commented Nov 10, 2017

djzin commented Nov 13, 2017

nikomatsakis commented Nov 14, 2017

nikomatsakis commented Nov 14, 2017

bors commented Nov 14, 2017

bors commented Nov 14, 2017

bors commented Nov 14, 2017

eddyb commented Nov 18, 2017 •

edited

Loading

eddyb Nov 18, 2017

djzin Nov 18, 2017

eddyb Nov 18, 2017

djzin Nov 18, 2017

always add an unreachable branch on matches to give more info to llvm #45821

always add an unreachable branch on matches to give more info to llvm #45821

Conversation

djzin commented Nov 7, 2017

rust-highfive commented Nov 7, 2017

nikomatsakis commented Nov 7, 2017

kennytm commented Nov 7, 2017 • edited Loading

nikomatsakis commented Nov 7, 2017

Choose a reason for hiding this comment

djzin Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djzin Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Nov 7, 2017

nikomatsakis commented Nov 7, 2017

djzin commented Nov 7, 2017 • edited Loading

djzin commented Nov 7, 2017 • edited Loading

nikomatsakis commented Nov 8, 2017

nikomatsakis commented Nov 10, 2017

djzin commented Nov 13, 2017

nikomatsakis commented Nov 14, 2017

nikomatsakis commented Nov 14, 2017

bors commented Nov 14, 2017

bors commented Nov 14, 2017

bors commented Nov 14, 2017

eddyb commented Nov 18, 2017 • 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

kennytm commented Nov 7, 2017 •

edited

Loading

djzin Nov 7, 2017 •

edited

Loading

djzin Nov 8, 2017 •

edited

Loading

djzin commented Nov 7, 2017 •

edited

Loading

djzin commented Nov 7, 2017 •

edited

Loading

eddyb commented Nov 18, 2017 •

edited

Loading