Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid unnecessary copies of arguments that are simple bindings #45380

Merged
merged 2 commits into from Oct 26, 2017

Conversation

@dotdash
Copy link
Contributor

@dotdash dotdash commented Oct 19, 2017

Initially MIR differentiated between arguments and locals, which
introduced a need to add extra copies assigning the argument to a
local, even for simple bindings. This differentiation no longer exists,
but we're still creating those copies, bloating the MIR and LLVM IR we
emit.

Additionally, the current approach means that we create debug info for
both the incoming argument (marking it as an argument), and then
immediately shadow it a local that goes by the same name. This can be
confusing when using e.g. "info args" in gdb, or when e.g. a debugger
with a GUI displays the function arguments separately from the local
variables, especially when the binding is mutable, because the argument
doesn't change, while the local variable does.

@rust-highfive
Copy link
Collaborator

@rust-highfive rust-highfive commented Oct 19, 2017

r? @arielb1

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

@dotdash
Copy link
Contributor Author

@dotdash dotdash commented Oct 19, 2017

@Mark-Simulacrum what do I need to get perf results for this?

@Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Oct 19, 2017

@bors try

Ping me when this completes, either here or on IRC and I'll run perf and get back to you.

@bors
Copy link
Contributor

@bors bors commented Oct 19, 2017

Trying commit b7fa03a with merge 0d49100...

bors added a commit that referenced this pull request Oct 19, 2017
Avoid unnecessary copies of arguments that are simple bindings

Initially MIR differentiated between arguments and locals, which
introduced a need to add extra copies assigning the argument to a
local, even for simple bindings. This differentiation no longer exists,
but we're still creating those copies, bloating the MIR and LLVM IR we
emit.

Additionally, the current approach means that we create debug info for
both the incoming argument (marking it as an argument), and then
immediately shadow it a local that goes by the same name. This can be
confusing when using e.g. "info args" in gdb, or when e.g. a debugger
with a GUI displays the function arguments separately from the local
variables, especially when the binding is mutable, because the argument
doesn't change, while the local variable does.
@bors
Copy link
Contributor

@bors bors commented Oct 19, 2017

☀️ Test successful - status-travis
State: approved= try=True

@Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Oct 19, 2017

Solid wins across the board timing-wise, though some of the memory use benchmarks increased by 5-15%: http://perf.rust-lang.org/compare.html?start=b7960878ba77124505aabe7dc99d0a898354c326&end=0d4910029c828c95069d88768882a0a46a19220d&stat=instructions%3Au.

@dotdash dotdash force-pushed the dotdash:arg_copies branch from b7fa03a to ac99309 Oct 20, 2017
@bluss bluss added the relnotes label Oct 20, 2017
@michaelwoerister
Copy link
Contributor

@michaelwoerister michaelwoerister commented Oct 23, 2017

Those performance improvements look awesome, @dotdash!

@arielb1
Copy link
Contributor

@arielb1 arielb1 commented Oct 24, 2017

So this doesn't break simd_shuffle? Might be worth checking out why.

By that, I mean this code:

if is_shuffle && idx == 2 {
match *arg {
mir::Operand::Consume(_) => {
span_bug!(span, "shuffle indices must be constant");
}
mir::Operand::Constant(ref constant) => {
let val = self.trans_constant(&bcx, constant);
llargs.push(val.llval);
idx += 1;
continue;
}
}
}

Which is tested in https://github.com/rust-lang/rust/blob/a789fa0440214347e1bf6228fdb8fd36bf2f4520/src/test/run-pass/simd-intrinsic-generic-elements.rs

@arielb1
Copy link
Contributor

@arielb1 arielb1 commented Oct 24, 2017

So there's a test, so let's

@bors r+

and if this bounces, we'll have our reason

@bors
Copy link
Contributor

@bors bors commented Oct 24, 2017

📌 Commit ac99309 has been approved by arielb1

@@ -247,7 +247,11 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
} else {
let args: Vec<_> =
args.into_iter()
.map(|arg| unpack!(block = this.as_local_operand(block, arg)))
.map(|arg| {

This comment has been minimized.

@arielb1

arielb1 Oct 24, 2017
Contributor

Actually, can we have a comment here that says that we must copy the argument to a temp, because functions are allowed to mutate arguments passed to them by-reference on the ABI level? And add a similar comment to librustc/mir/mod.rs so that there is no confusion?

This comment has been minimized.

@eddyb

eddyb Nov 13, 2017
Member

I'm not sure I like the resulting state from this... I think that Call should either take Locals instead of Operands, or trans should generate the copy out of non-immediate constants.

This comment has been minimized.

@eddyb

eddyb Nov 13, 2017
Member

In fact, do we really want ABI concerns to shape MIR itself, as opposed to the ability to analyze it? I came back to this because I noticed that calls no longer had constants directly in them and thus copy propagation is incorrect into calls, and, without it, certain analyses would now need a bit more dataflow to even observe plain constants in function arguments.

This comment has been minimized.

@eddyb

eddyb Nov 14, 2017
Member

Talked this over with @arielb1 on IRC and I'm leaning towards "Operand::Constant is caller-owned, Operand::Consume is callee-owned", which would remove the temporaries for constant arguments and facilitate

Furthermore, we can skip some copies (for !Copy arguments) during MIR construction and have MIR borrowck enforce the soundness of Calls (lock the return destination and move in all the arguments).

@arielb1
Copy link
Contributor

@arielb1 arielb1 commented Oct 24, 2017

And reading this again, I think that the "ABI detail" of function arguments belonging to the callee should be documented, at least at librustc/mir/mod.rs but probably elsewhere too.

@bors r-

@dotdash
Copy link
Contributor Author

@dotdash dotdash commented Oct 25, 2017

So this doesn't break simd_shuffle? Might be worth checking out why.

QualifyConsts and PromoteConsts special case simd_shuffle, so constant promotion still kicks in there.

pub enum Candidate {
/// Borrow of a constant temporary.
Ref(Location),
/// Array of indices found in the third argument of
/// a call to one of the simd_shuffleN intrinsics.
ShuffleIndices(BasicBlock)
}

@dotdash
Copy link
Contributor Author

@dotdash dotdash commented Oct 25, 2017

Added some comments to clarify the ownership of function arguments.

@arielb1
Copy link
Contributor

@arielb1 arielb1 commented Oct 25, 2017

@bors r+

@bors
Copy link
Contributor

@bors bors commented Oct 25, 2017

📌 Commit 1cc57cd has been approved by arielb1

@bors
Copy link
Contributor

@bors bors commented Oct 25, 2017

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

@dotdash dotdash force-pushed the dotdash:arg_copies branch from 1cc57cd to f4e8432 Oct 25, 2017
@dotdash
Copy link
Contributor Author

@dotdash dotdash commented Oct 25, 2017

@bors r=arielb1

dotdash added 2 commits Oct 11, 2017
Initially MIR differentiated between arguments and locals, which
introduced a need to add extra copies assigning the argument to a
local, even for simple bindings. This differentiation no longer exists,
but we're still creating those copies, bloating the MIR and LLVM IR we
emit.

Additionally, the current approach means that we create debug info for
both the incoming argument (marking it as an argument), and then
immediately shadow it a local that goes by the same name. This can be
confusing when using e.g. "info args" in gdb, or when e.g. a debugger
with a GUI displays the function arguments separately from the local
variables, especially when the binding is mutable, because the argument
doesn't change, while the local variable does.
@dotdash dotdash force-pushed the dotdash:arg_copies branch from f4e8432 to 8ad7c28 Oct 26, 2017
@dotdash
Copy link
Contributor Author

@dotdash dotdash commented Oct 26, 2017

@bors r=arielb1

@bors
Copy link
Contributor

@bors bors commented Oct 26, 2017

📌 Commit 8ad7c28 has been approved by arielb1

@bors
Copy link
Contributor

@bors bors commented Oct 26, 2017

Testing commit 8ad7c28 with merge 7f8fe16...

bors added a commit that referenced this pull request Oct 26, 2017
Avoid unnecessary copies of arguments that are simple bindings

Initially MIR differentiated between arguments and locals, which
introduced a need to add extra copies assigning the argument to a
local, even for simple bindings. This differentiation no longer exists,
but we're still creating those copies, bloating the MIR and LLVM IR we
emit.

Additionally, the current approach means that we create debug info for
both the incoming argument (marking it as an argument), and then
immediately shadow it a local that goes by the same name. This can be
confusing when using e.g. "info args" in gdb, or when e.g. a debugger
with a GUI displays the function arguments separately from the local
variables, especially when the binding is mutable, because the argument
doesn't change, while the local variable does.
@bors
Copy link
Contributor

@bors bors commented Oct 26, 2017

💔 Test failed - status-appveyor

@kennytm
Copy link
Member

@kennytm kennytm commented Oct 26, 2017

@bors retry

xsv's prop_frequency panicked on cargotest x86_64-pc-windows-msvc. According to #45348 (comment) this failure is spurious. cc @BurntSushi.

thread 'test_frequency::prop_frequency' panicked at '[quickcheck] TEST FAILED (runtime error). Arguments: (CsvData { data: [[[239, 187, 191]], [[]]] })

Edit: Looks like this spurious bug has been fixed in BurntSushi/xsv@92de288. We just need to update our cargotest hash.

kennytm added a commit to kennytm/rust that referenced this pull request Oct 26, 2017
This fixes a flaky test which caused spurious failures in rust-lang#45348 and rust-lang#45380
@kennytm kennytm mentioned this pull request Oct 26, 2017
@bors
Copy link
Contributor

@bors bors commented Oct 26, 2017

Testing commit 8ad7c28 with merge b0b80f8...

bors added a commit that referenced this pull request Oct 26, 2017
Avoid unnecessary copies of arguments that are simple bindings

Initially MIR differentiated between arguments and locals, which
introduced a need to add extra copies assigning the argument to a
local, even for simple bindings. This differentiation no longer exists,
but we're still creating those copies, bloating the MIR and LLVM IR we
emit.

Additionally, the current approach means that we create debug info for
both the incoming argument (marking it as an argument), and then
immediately shadow it a local that goes by the same name. This can be
confusing when using e.g. "info args" in gdb, or when e.g. a debugger
with a GUI displays the function arguments separately from the local
variables, especially when the binding is mutable, because the argument
doesn't change, while the local variable does.
@bors
Copy link
Contributor

@bors bors commented Oct 26, 2017

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

@bors bors merged commit 8ad7c28 into rust-lang:master Oct 26, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Oct 28, 2017

This improved the debug compilation time of the regex crate by almost 8%, nice!

bors added a commit that referenced this pull request Oct 31, 2017
cargotest: Update xsv.

This fixes a flaky test which caused spurious failures in #45348 and #45380.
bors added a commit that referenced this pull request Nov 17, 2017
MIR: hide .rodata constants vs by-ref ABI clash in trans.

Back in #45380, constants were copied into locals during MIR creation to ensure that arguments ' memory can be used by the callee, if the constant is placed in `.rodata` and the ABI passes it by-ref.

However, there are several drawbacks (see #45380 (comment)), most importantly the complication of constant propagation (UB if a constant ends up in `Call` arguments) and inconveniencing analyses.

Instead, I've modified the `rustc_trans` implementation of calls to copy an `Operand::Constant` argument locally if it's not immediate, and added a test that segfaults without the copy.

cc @dotdash @arielb1
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Dec 3, 2017
Fix CopyPropagation regression (2)

Remaining part of MIR copyprop regression by (I think) rust-lang#45380, which I missed in rust-lang#45753.

```rust
fn foo(mut x: i32) -> i32 {
    let y = x;
    x = 123; // `x` is assigned only once in MIR, but cannot be propagated to `y`
    y
}
```

So any assignment to an argument cannot be propagated.
@dotdash dotdash deleted the dotdash:arg_copies branch Jan 31, 2018
@philipc philipc mentioned this pull request Jan 13, 2019
2 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.