Various improvements to MIR and LLVM IR Construction #32980

Aatch · 2016-04-15T00:57:04Z

Primarily affects the MIR construction, which indirectly improves LLVM
IR generation, but some LLVM IR changes have been made too.

Handle "statement expressions" more intelligently. These are
expressions that always evaluate to (). Previously a temporary would
be generated as a destination to translate into, which is unnecessary.

This affects assignment, augmented assignment, return, break and
continue.
Avoid inserting drops for non-drop types in more places. Scheduled
drops were already skipped for types that we knew wouldn't need
dropping at construction time. However manually-inserted drops like
those for x in x = y; were still generated. build_drop now takes
a type parameter like its schedule_drop counterpart and checks to
see if the type needs dropping.
Avoid generating an extra temporary for an assignment where the types
involved don't need dropping. Previously an expression like
a = b + 1; would result in a temporary for b + 1. This is so the
RHS can be evaluated, then the LHS evaluated and dropped and have
everything work correctly. However, this isn't necessary if the LHS
doesn't need a drop, as we can just overwrite the existing value.
Improves lvalue analysis to allow treating an Rvalue::Use as an
operand in certain conditions. The reason for it never being an
operand is so it can be zeroed/drop-filled, but this is only true for
types that need dropping.

The first two changes result in significantly fewer MIR blocks being
generated, as previously almost every statement would end up generating
a new block due to the drop of the () temporary being generated.

rust-highfive · 2016-04-15T00:57:15Z

r? @pnkfelix

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

Aatch · 2016-04-15T00:59:45Z

A comparison of the MIR and LLVM IR output before and after these changes: https://gist.github.com/Aatch/5257da7c18fb7a86595080c0851fabd5.

Which is for this code:

pub fn foo() -> i32 {
    let mut a = 0;
    while a < 100 {
        a += 1;

        a = a * 2;
    }

    a * a
}

I haven't made any measurements regarding compile times.

nagisa · 2016-04-15T17:46:51Z

src/librustc_trans/mir/rvalue.rs

@@ -599,7 +608,8 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
    }
 }

-pub fn rvalue_creates_operand<'tcx>(rvalue: &mir::Rvalue<'tcx>) -> bool {
+pub fn rvalue_creates_operand<'bcx, 'tcx>(mir: &mir::Mir<'tcx>, bcx: Block<'bcx, 'tcx>,


Pass a BlockAndBuilder here and use its tcx and monomorphize methods instead. bcx.with_block does work to make sure blocks are in suitable condition for old-trans and the method should be avoided at all costs unless you’re calling back into old-trans machinery.

nagisa · 2016-04-15T18:24:08Z

The improvements look very nice. I left some comments, questions and nits inline.

r? @nagisa

arielb1 · 2016-04-15T20:46:56Z

The original plan was to do these changes in a post-optimization pass.

nagisa · 2016-04-15T21:03:26Z

The original plan was to do these changes in a post-optimization pass.

Some of them, yes. We will still end up having a pass removing unused temps/vars etc for example, so the passes wouldn’t need to bother with them much, but feeding cleaner MIR to the passes in the first place is also a good idea, especially, when its such a low-hanging fruit (in terms of diff) as it is in this case.

Aatch · 2016-04-16T07:49:17Z

Comments addressed and a bug fixed I found locally.

nagisa · 2016-04-16T08:50:28Z

src/librustc_trans/mir/mod.rs

@@ -108,6 +108,16 @@ enum TempRef<'tcx> {
    Operand(Option<OperandRef<'tcx>>),
 }

+impl<'tcx> TempRef<'tcx> {
+    fn new_operand(val: OperandValue, ty: ty::Ty<'tcx>) -> TempRef<'tcx> {


I was thinking that it would check for ty being zero-sized and returned either the wrapped C_nil or None, like the whole else-if-else block did in the original commit, because that’s the kinda of an important assumption for code elsewhere to work (EDIT: namely, the changes in trans/mir/statement).

Ah, that makes sense.

Aatch · 2016-04-17T02:49:26Z

Travis is finding more of those codegen-unit failures related to drop-glue i8. I'll fix those a little later. Otherwise I made the change to new_operand, @nagisa

Aatch · 2016-04-17T04:37:59Z

Codegen-units tests fixed, though I'm still not sure what the significance of drop-glue i8 is.

nagisa · 2016-04-17T13:37:17Z

Codegen-units tests fixed, though I'm still not sure what the significance of drop-glue i8 is.

These are generated from depgraph which collects data from MIR. It seems correct to me that some of these items disappear, because we do emit less drops (and thus sometimes drop glue becomes unnecessary) now.

nagisa · 2016-04-17T13:41:27Z

@bors r+ d574d56

bors · 2016-04-17T14:45:47Z

⌛ Testing commit d574d56 with merge 8d4ef4c...

bors · 2016-04-17T16:09:23Z

💔 Test failed - auto-linux-64-opt-mir

nagisa · 2016-04-17T17:03:48Z

The test failure seems to be non-spurious. This runner likely runs in the -Z orbit mode.

Aatch · 2016-04-19T00:08:57Z

Ah, the handling of closures caused it to try and translate an operand temp as an lvalue, which triggered the assertion.

Aatch · 2016-04-20T06:14:34Z

Basically re-wrote trans_arguments_untupled. @eddyb want to double-check my changes there?

eddyb · 2016-04-20T07:07:19Z

src/librustc_trans/mir/block.rs

+            }
+            Immediate(llval) => {
+                for (n, &ty) in arg_types.iter().enumerate() {
+                    let mut elem = bcx.extract_value(llval, n);


Couldn't this be abstracted away in a single place?

Probably, I'd like to rework most of how we deal with ADTs though, especially now that #32939 has landed.

arielb1 · 2016-04-20T18:28:27Z

@Aatch

What was the problem with the old version? The alloca should not really be used in practice, and could be removed with a better const-eval.

nagisa · 2016-04-20T18:48:25Z

src/librustc_trans/mir/block.rs

-                Ref(ptr)
-            };
-            self.trans_argument(bcx, val, llargs, fn_ty, next_idx, callee);
+        // Handle both by-ref and immediate tuples. This gives us the option of


NIT: incomplete comment?

nagisa · 2016-04-20T18:55:28Z

@arielb1

The alloca should not really be used in practice, and could be removed with a better const-eval.

What alloca are you referring to? Before this PR, pretty much everything is an alloca-backed Lvalue.

arielb1 · 2016-04-20T20:33:52Z

@nagisa

I missed the extractvalue trick, which is required with trans_operand.

bors · 2016-04-21T11:44:11Z

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

Primarily affects the MIR construction, which indirectly improves LLVM IR generation, but some LLVM IR changes have been made too. * Handle "statement expressions" more intelligently. These are expressions that always evaluate to `()`. Previously a temporary would be generated as a destination to translate into, which is unnecessary. This affects assignment, augmented assignment, `return`, `break` and `continue`. * Avoid inserting drops for non-drop types in more places. Scheduled drops were already skipped for types that we knew wouldn't need dropping at construction time. However manually-inserted drops like those for `x` in `x = y;` were still generated. `build_drop` now takes a type parameter like its `schedule_drop` counterpart and checks to see if the type needs dropping. * Avoid generating an extra temporary for an assignment where the types involved don't need dropping. Previously an expression like `a = b + 1;` would result in a temporary for `b + 1`. This is so the RHS can be evaluated, then the LHS evaluated and dropped and have everything work correctly. However, this isn't necessary if the `LHS` doesn't need a drop, as we can just overwrite the existing value. * Improves lvalue analysis to allow treating an `Rvalue::Use` as an operand in certain conditions. The reason for it never being an operand is so it can be zeroed/drop-filled, but this is only true for types that need dropping. The first two changes result in significantly fewer MIR blocks being generated, as previously almost every statement would end up generating a new block due to the drop of the `()` temporary being generated.

Moves `stmt_expr` into its own module, `expr::stmt`.

The drop glue for `i8` is no longer generated as a trans item

In code like `let x = y = z;`, `y = z` goes through `as_rvalue`, which didn't handle it. Now it translates the assignment and produces `()` directly.

`new_operand` now checks the type it's given and either creates the nil value itself, or produces an empty operand.

I'm not sure what the signficance of `drop-glue i8` is, nor why one of the tests had it appear while the others had it disappear. Either way it doesn't seem like the presence or absense of it is the focus of the tests.

Use either getelementptr or extractvalue depending on whether or not the tuple is immediate or not.

LLVM's assertion doesn't provide much insight as to what the problem was. We were already checking `call` instructions ourselves, so this brings the checks from there to `invoke`. Both the `invoke` and `call` checking is controlled by `debug_assertions`.

The logic for checking `call` and `invoke` instructions was duplicated between them, so factor it out to a helper method.

Aatch · 2016-04-28T02:09:23Z

Rebased, sorry for the delay on that.

nagisa · 2016-04-28T07:20:42Z

@bors r±
On Apr 28, 2016 5:10 AM, "James Miller" notifications@github.com wrote:

Rebased, sorry for the delay on that.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#32980 (comment)

Aatch · 2016-04-28T07:27:17Z

@bors r=nagisa

I think something got messed up with your email.

bors · 2016-04-28T07:27:53Z

📌 Commit 5bda576 has been approved by nagisa

bors · 2016-04-28T08:26:45Z

⌛ Testing commit 5bda576 with merge 009a649...

Various improvements to MIR and LLVM IR Construction Primarily affects the MIR construction, which indirectly improves LLVM IR generation, but some LLVM IR changes have been made too. * Handle "statement expressions" more intelligently. These are expressions that always evaluate to `()`. Previously a temporary would be generated as a destination to translate into, which is unnecessary. This affects assignment, augmented assignment, `return`, `break` and `continue`. * Avoid inserting drops for non-drop types in more places. Scheduled drops were already skipped for types that we knew wouldn't need dropping at construction time. However manually-inserted drops like those for `x` in `x = y;` were still generated. `build_drop` now takes a type parameter like its `schedule_drop` counterpart and checks to see if the type needs dropping. * Avoid generating an extra temporary for an assignment where the types involved don't need dropping. Previously an expression like `a = b + 1;` would result in a temporary for `b + 1`. This is so the RHS can be evaluated, then the LHS evaluated and dropped and have everything work correctly. However, this isn't necessary if the `LHS` doesn't need a drop, as we can just overwrite the existing value. * Improves lvalue analysis to allow treating an `Rvalue::Use` as an operand in certain conditions. The reason for it never being an operand is so it can be zeroed/drop-filled, but this is only true for types that need dropping. The first two changes result in significantly fewer MIR blocks being generated, as previously almost every statement would end up generating a new block due to the drop of the `()` temporary being generated.

bors · 2016-04-28T10:38:02Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-32cross-opt, auto-linux-64-cargotest, auto-linux-64-cross-armhf, auto-linux-64-cross-armsf, auto-linux-64-cross-freebsd, auto-linux-64-cross-netbsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, 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-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-cross-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild

rust-highfive assigned pnkfelix Apr 15, 2016

nagisa reviewed Apr 15, 2016
View reviewed changes

rust-highfive assigned nagisa and unassigned pnkfelix Apr 15, 2016

Aatch force-pushed the better-mir-building branch from 47a3ad1 to ad91f9f Compare April 16, 2016 06:11

nagisa reviewed Apr 16, 2016
View reviewed changes

Aatch force-pushed the better-mir-building branch from 820d1ef to 82d933e Compare April 17, 2016 02:45

eddyb reviewed Apr 20, 2016
View reviewed changes

nagisa reviewed Apr 20, 2016
View reviewed changes

Aatch added 9 commits April 28, 2016 13:17

Address comments

c2de80f

Moves `stmt_expr` into its own module, `expr::stmt`.

Fixup tests

8691723

The drop glue for `i8` is no longer generated as a trans item

Fix translation of Assign/AssignOp as rvalues

89edd96

In code like `let x = y = z;`, `y = z` goes through `as_rvalue`, which didn't handle it. Now it translates the assignment and produces `()` directly.

Move zero-sized type handling logic to new_operand

c55d9e5

`new_operand` now checks the type it's given and either creates the nil value itself, or produces an empty operand.

Fix codegen-units tests

0e3b37a

I'm not sure what the signficance of `drop-glue i8` is, nor why one of the tests had it appear while the others had it disappear. Either way it doesn't seem like the presence or absense of it is the focus of the tests.

Handle immediate tuples in trans_arguments_untupled

3bcee26

Use either getelementptr or extractvalue depending on whether or not the tuple is immediate or not.

Factor out function call checking to a helper method

5bda576

The logic for checking `call` and `invoke` instructions was duplicated between them, so factor it out to a helper method.

Aatch force-pushed the better-mir-building branch from 71665e7 to 5bda576 Compare April 28, 2016 02:07

bors merged commit 5bda576 into rust-lang:master Apr 28, 2016

bors mentioned this pull request Apr 28, 2016

Remove unnecesary "as" casts. #32701

Closed

sanxiyn mentioned this pull request May 4, 2016

MIR: Bootstrap up to a half-hour longer with --enable-orbit #33111

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various improvements to MIR and LLVM IR Construction #32980

Various improvements to MIR and LLVM IR Construction #32980

Aatch commented Apr 15, 2016

rust-highfive commented Apr 15, 2016

Aatch commented Apr 15, 2016 •

edited

Loading

nagisa Apr 15, 2016 •

edited

Loading

nagisa commented Apr 15, 2016

arielb1 commented Apr 15, 2016

nagisa commented Apr 15, 2016 •

edited

Loading

Aatch commented Apr 16, 2016

nagisa Apr 16, 2016 •

edited

Loading

Aatch Apr 16, 2016

Aatch commented Apr 17, 2016

Aatch commented Apr 17, 2016

nagisa commented Apr 17, 2016 •

edited

Loading

nagisa commented Apr 17, 2016

bors commented Apr 17, 2016

bors commented Apr 17, 2016

nagisa commented Apr 17, 2016

Aatch commented Apr 19, 2016

Aatch commented Apr 20, 2016

eddyb Apr 20, 2016

Aatch Apr 20, 2016

arielb1 commented Apr 20, 2016 •

edited

Loading

nagisa Apr 20, 2016

nagisa commented Apr 20, 2016

arielb1 commented Apr 20, 2016

bors commented Apr 21, 2016

Aatch commented Apr 28, 2016

nagisa commented Apr 28, 2016

Aatch commented Apr 28, 2016

bors commented Apr 28, 2016

bors commented Apr 28, 2016

bors commented Apr 28, 2016

Various improvements to MIR and LLVM IR Construction #32980

Various improvements to MIR and LLVM IR Construction #32980

Conversation

Aatch commented Apr 15, 2016

rust-highfive commented Apr 15, 2016

Aatch commented Apr 15, 2016 • edited Loading

nagisa Apr 15, 2016 • edited Loading

Choose a reason for hiding this comment

nagisa commented Apr 15, 2016

arielb1 commented Apr 15, 2016

nagisa commented Apr 15, 2016 • edited Loading

Aatch commented Apr 16, 2016

nagisa Apr 16, 2016 • edited Loading

Choose a reason for hiding this comment

Aatch Apr 16, 2016

Choose a reason for hiding this comment

Aatch commented Apr 17, 2016

Aatch commented Apr 17, 2016

nagisa commented Apr 17, 2016 • edited Loading

nagisa commented Apr 17, 2016

bors commented Apr 17, 2016

bors commented Apr 17, 2016

nagisa commented Apr 17, 2016

Aatch commented Apr 19, 2016

Aatch commented Apr 20, 2016

eddyb Apr 20, 2016

Choose a reason for hiding this comment

Aatch Apr 20, 2016

Choose a reason for hiding this comment

arielb1 commented Apr 20, 2016 • edited Loading

nagisa Apr 20, 2016

Choose a reason for hiding this comment

nagisa commented Apr 20, 2016

arielb1 commented Apr 20, 2016

bors commented Apr 21, 2016

Aatch commented Apr 28, 2016

nagisa commented Apr 28, 2016

Aatch commented Apr 28, 2016

bors commented Apr 28, 2016

bors commented Apr 28, 2016

bors commented Apr 28, 2016

Aatch commented Apr 15, 2016 •

edited

Loading

nagisa Apr 15, 2016 •

edited

Loading

nagisa commented Apr 15, 2016 •

edited

Loading

nagisa Apr 16, 2016 •

edited

Loading

nagisa commented Apr 17, 2016 •

edited

Loading

arielb1 commented Apr 20, 2016 •

edited

Loading