Fix optimization regressions for operations on [x; n]-initialized arrays. by eddyb · Pull Request #36124 · rust-lang/rust

eddyb · 2016-08-29T23:51:39Z

Fixes#35662 by using != instead of < as the stop condition for [x; n] initialization loops.
Also included is eddyb/llvm@cc2009f, a hack to run the GVN pass twice, another time after InstCombine.
This hack results in removal of redundant memset and memcpy calls (from loops over arrays).

cc @nrc Can we get performance numbers on this? Not sure if it regresses anything else.

rust-highfive · 2016-08-29T23:51:42Z

r? @aturon

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

bors · 2016-08-30T04:35:03Z

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

bluss · 2016-08-30T05:27:54Z

Nice find. It should be much better this way, the optimizer prefers < / <= for an integer-counted loop and != for a pointer loop.

eddyb · 2016-08-30T05:50:42Z

@bluss You sure? Overflow shenanigans apply to both pointers and integers.
With !=, the distance for a monotonic loop is guaranteed to be end - start.
But with < or <= you don't know the end directly, you have to infer it (and sometimes you can't).
You can't even know if it loops at least once!
That is, start != start + len iff len != 0 whereas start < start + len depends on start too.
In our case, it would be UB to cross the end of the address space, but still, it's more complicated.

eddyb · 2016-08-30T23:09:28Z

On IRC @nrc seems to think that it's easier to see what perf impact this has by getting it merged.
Also, @arielb1 mentioned that GVN is so fast we likely won't notice any change at all.

r? @alexcrichton

alexcrichton · 2016-08-31T16:21:52Z

Ok I've merged the LLVM commit, @eddyb wanna update the commit here?

…ays.

alexcrichton · 2016-08-31T21:56:19Z

@bors: r+

bors · 2016-08-31T21:56:21Z

📌 Commit f5c7752 has been approved by alexcrichton

bors · 2016-09-02T15:28:35Z

⌛ Testing commit f5c7752 with merge 2c7fd85...

bors · 2016-09-02T15:31:00Z

💔 Test failed - auto-win-gnu-32-opt-rustbuild

alexcrichton · 2016-09-02T15:56:51Z

@bors: retry

On Fri, Sep 2, 2016 at 8:31 AM, bors notifications@github.com wrote:

💔 Test failed - auto-win-gnu-32-opt-rustbuild
https://buildbot.rust-lang.org/builders/auto-win-gnu-32-opt-rustbuild/builds/2381
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#36124 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95L16oxTmyMa_205S9Kn-zbW4feQPks5qmEE3gaJpZM4JwAzX
.

bors · 2016-09-04T00:41:08Z

⌛ Testing commit f5c7752 with merge 1ca1de6...

@nrc

Fix optimization regressions for operations on [x; n]-initialized arrays. Fixes#35662 by using `!=` instead of `<` as the stop condition for `[x; n]` initialization loops. Also included is eddyb/llvm@cc2009f, a hack to run the GVN pass twice, another time after InstCombine. This hack results in removal of redundant `memset` and `memcpy` calls (from loops over arrays). cc @nrc Can we get performance numbers on this? Not sure if it regresses anything else.

bors · 2016-09-04T04:56:32Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, 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-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-rustbuild
Approved by: alexcrichton
Pushing 1ca1de6 to master...

eddyb · 2016-09-08T12:43:04Z

@alexcrichton @nrc @nikomatsakis This caused a 4% regression in bootstrap. Hopefully it's worth it.

nikomatsakis · 2016-09-19T20:46:30Z

The patch is tiny. I think it's worth backporting. I don't know what to make of the 4% regression. Could be a lot of things.

cc @rust-lang/compiler

pnkfelix · 2016-09-21T11:24:31Z

From skimming the perf benchmarks (rather than bootstrap) times, I think we can accept the risk associated with backporting this.

~~(There is a weird regression of the piston-image-0.10.3 benchmark, but if I understand the graph correctly, that is due to a commit that came after this PR...)~~

Never mind this note; the timing for piston-image-0.10.3 is zero before this point, which I interpret as meaning that it wasn't actually being run at all.
@nrc if you can, you may want to consider not rendering the points that are assigned a time of zero.

nagisa · 2016-09-21T15:15:31Z

 let current = Phi(header_bcx, val_ty(start), &[start], &[bcx.llbb]);

 let keep_going =
- ICmp(header_bcx, llvm::IntULT, current, end, DebugLoc::None);


This is the only line that has to be applied to beta for backport.

Without the LLVM change the regression isn't fully fixed, but yeah, this is the main part of it.
It might be in different places, so searching for IntULT used in an iteration loop condition should help.

Beta is on LLVM commit further ahead now due to previous backport.

nikomatsakis · 2016-09-21T15:24:46Z

OK, I will go ahead and mark as beta-accepted. Backporter should note that backport can be quite minimum, per @nagisa's comments here.

brson · 2016-09-21T19:29:08Z

I've started a backport.

rust-highfive assigned aturon Aug 29, 2016

eddyb mentioned this pull request Aug 30, 2016
Run GVN again after InstCombine. rust-lang/llvm#49
Merged

eddyb force-pushed the fast-array-init branch from 800c1ee to a4cc02fCompare August 30, 2016 23:07

rust-highfive assigned alexcrichton and unassigned aturon Aug 30, 2016

Fix optimization regressions for operations on [x; n]-initialized arr…
f5c7752
…ays.

eddyb force-pushed the fast-array-init branch from a4cc02f to f5c7752Compare August 31, 2016 21:27

eddyb mentioned this pull request Aug 31, 2016
LLVM pointer range loop / autovectorization regression #35662
Closed

nikomatsakis mentioned this pull request Sep 1, 2016
Update LLVM to include 4 backported commits by @majnemer. #35740
Merged

eddyb added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 1, 2016

bors merged commit f5c7752 into rust-lang:masterSep 4, 2016

eddyb deleted the fast-array-init branch September 4, 2016 04:57

brson mentioned this pull request Sep 19, 2016
Beta backports #36591
Merged

pnkfelix mentioned this pull request Sep 21, 2016
Beta backports #36620
Merged

nagisa reviewed Sep 21, 2016
View reviewed changes

nikomatsakis added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Sep 21, 2016

brson mentioned this pull request Sep 21, 2016
[beta] Fix optimization regressions for operations on [x; n]-initialized arr… #36634
Merged

brson removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 21, 2016

Uh oh!

Uh oh!

Conversation

eddyb commented Aug 29, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Aug 29, 2016

Uh oh!

bors commented Aug 30, 2016

Uh oh!

bluss commented Aug 30, 2016

Uh oh!

eddyb commented Aug 30, 2016

Uh oh!

eddyb commented Aug 30, 2016

Uh oh!

alexcrichton commented Aug 31, 2016

Uh oh!

alexcrichton commented Aug 31, 2016

Uh oh!

bors commented Aug 31, 2016

Uh oh!

bors commented Sep 2, 2016

Uh oh!

bors commented Sep 2, 2016

Uh oh!

alexcrichton commented Sep 2, 2016

Uh oh!

bors commented Sep 4, 2016

Uh oh!

bors commented Sep 4, 2016

Uh oh!

eddyb commented Sep 8, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikomatsakis commented Sep 19, 2016

Uh oh!

pnkfelix commented Sep 21, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nagisaSep 21, 2016

Choose a reason for hiding this comment

Uh oh!

eddybSep 21, 2016

Choose a reason for hiding this comment

Uh oh!

nagisaSep 21, 2016

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Sep 21, 2016

Uh oh!

brson commented Sep 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

eddyb commented Aug 29, 2016•
edited
Loading

eddyb commented Sep 8, 2016•
edited
Loading

pnkfelix commented Sep 21, 2016•
edited
Loading