[mir-opt] asking ?s in a more optimized fashion by Centril · Pull Request #66282 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation61 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Centril

This PR works towards #66234 by providing two optimization passes meant to run in sequence:

Together, these are meant to simplify the following into just res:

match res { Ok(x) => Ok(x), Err(x) => Err(x), }

It should be noted however that the desugaring of ? includes a function call and so the first pass in this PR relies on inlining to substitute that function call for identity on x. Inlining requires mir-opt-level=2 so this might not have any effect in perf-bot but let's find out.

r? @oli-obk -- This is WIP, but I'd appreciate feedback. :)

@Centril

@rust-timer

Awaiting bors try build completion

@bors

⌛ Trying commit 91e5a0e with merge fb1a35ea56157ef7f76c1451f72cda0b3be86a38...

@Centril

@bors bors 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 11, 2019

@Centril

@bors

bors added a commit that referenced this pull request

Nov 11, 2019

@bors

[WIP] [mir-opt] asking ?s in a more optimized fashion

This PR works towards #66234 by providing two optimization passes meant to run in sequence:

Together, these are meant to simplify the following into just res:

match res {
    Ok(x) => Ok(x),
    Err(x) => Err(x),
}

It should be noted however that the desugaring of ? includes a function call and so the first pass in this PR relies on inlining to substitute that function call for identity on x. Inlining requires mir-opt-level=2 so this might not have any effect in perf-bot but let's find out.

r? @oli-obk -- This is WIP, but I'd appreciate feedback. :)

Centril

// scope 6 {
// }
// bb0: {
// _2 = discriminant(_1);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a tad unfortunate that _2 isn't optimized out.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we had some sort of DCE. If not, open an issue so we create it

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will be easy to fix. I'll open a PR tonight with that.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some ppl at rustfest working on it

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that refer to #66329 ? -- That won't help in this case since this is an unused, but reachable, local.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, different optimization, not sure on what repo it's being developed

Centril

Centril

Centril

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@bors

☀️ Try build successful - checks-azure
Build commit: 0113342 (0113342691c2ecbb2b7fd1d083fd0631f136b1dd)

@rust-timer

oli-obk

@rust-timer

@rust-highfive

This comment has been minimized.

@Centril Centril changed the title[WIP] [mir-opt] asking ?s in a more optimized fashion [mir-opt] asking ?s in a more optimized fashion

Nov 12, 2019

@Centril

Adjusted the existing match.rs codegen test and introduced a new one which demonstrates that the MIR optimizations introduced produce better LLVM IR as compared to before.

Me and Oliver have been discussing some generalizations to these optimizations (for follow ups). In particular, we can generalize the second one into two passes:

  1. DedupBlocks -- This one looks for identical basic blocks, collects a substitution map, and then substitutes all references to BasicBlock for the key in the map. If we want, this could be run to a fixed point, but we could start it off as a one-shot pass.
  2. SwitchToSamePlace -- This does (could be split up but maybe not depending on complexity):
    a. Filters BBs that are unreachable
    b. Merges identical targets
    c. If one target is left ==> goto
    d. If no targets are left ==> unreachable
    -- a. & d. sorta want to be run to a fixed point but we should probably start off as one-shot instead.

Finally, the SimplifyArmIdentity could be generalized to also recognize e.g. Var(a, b, c) => Var(a, b, c) and not just Var(a) => Var(a).

@Centril

Actually, looking at CfgSimplifier::simplify_branch it looks like SwitchToSamePlace is a generalization of that logic. simplify_branch requires all targets to end up in the same place whereas the generalization would first do a filter and simplify e.g. -> [bb2, bb3, otherwise: bb2] to -> [bb2, otherwise: bb3]. We can perhaps add the logic there instead.

@Centril Centril added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

labels

Nov 12, 2019

@Centril

This should be ready now :)

@bors bors 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-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Nov 21, 2019

@bors

⌛ Testing commit 2f00e86 with merge ae731e69b0e519dbf73d4a870a82224b778808de...

@rust-highfive

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log. 

2019-11-22T00:10:43.0377317Z do so (now or later) by using -b with the checkout command again. Example:
2019-11-22T00:10:43.0378736Z 
2019-11-22T00:10:43.0378832Z   git checkout -b <new-branch-name>
2019-11-22T00:10:43.0378867Z 
2019-11-22T00:10:43.0378942Z HEAD is now at ae731e69b Auto merge of #66282 - Centril:simplify-try, r=oli-obk
2019-11-22T00:10:43.0769463Z ##[section]Starting: Decide whether to run this job
2019-11-22T00:10:43.0870334Z ==============================================================================
2019-11-22T00:10:43.0870455Z Task         : Bash
2019-11-22T00:10:43.0870535Z Description  : Run a Bash script on macOS, Linux, or Windows
---
2019-11-22T00:10:44.1732643Z 
2019-11-22T00:10:44.1732668Z 
2019-11-22T00:10:44.1732692Z 
2019-11-22T00:10:44.1732733Z 
2019-11-22T00:10:44.1732799Z     Err(x) => Err(x),
2019-11-22T00:10:44.1732868Z     Ok(x) => Ok(x),
2019-11-22T00:10:44.1732929Z   ((_LOCAL_0 as Variant).FIELD: TY) = move _LOCAL_TMP;
2019-11-22T00:10:44.1733006Z   _LOCAL_0 = move _LOCAL_1
2019-11-22T00:10:44.1733069Z   _LOCAL_TMP = ((_LOCAL_1 as Variant ).FIELD: TY );
2019-11-22T00:10:44.1733140Z   ```
2019-11-22T00:10:44.1733182Z   ```
2019-11-22T00:10:44.1733241Z   ```rust
2019-11-22T00:10:44.1733326Z   ```rust
2019-11-22T00:10:44.1733373Z   discriminant(_LOCAL_0) = VAR_IDX;
2019-11-22T00:10:44.1733598Z   into:
2019-11-22T00:10:44.1733653Z - `SimplifyArmIdentity` which transforms something like:
2019-11-22T00:10:44.1733752Z - `SimplifyBranchSame` which transforms `SwitchInt`s to identical basic blocks into a `goto` to the first reachable target.
2019-11-22T00:10:44.1733902Z AGENT_DISABLELOGPLUGIN_TESTFILEPUBLISHERPLUGIN=true
2019-11-22T00:10:44.1733982Z AGENT_DISABLELOGPLUGIN_TESTRESULTLOGPLUGIN=true
2019-11-22T00:10:44.1734038Z AGENT_HOMEDIRECTORY=C:\agents\2.160.1
2019-11-22T00:10:44.1734106Z AGENT_ID=520
---
2019-11-22T00:10:44.1741938Z BUILD_SOURCEBRANCHNAME=auto
2019-11-22T00:10:44.1742005Z BUILD_SOURCESDIRECTORY=D:\a\1\s
2019-11-22T00:10:44.1742068Z BUILD_SOURCEVERSION=ae731e69b0e519dbf73d4a870a82224b778808de
2019-11-22T00:10:44.1742142Z BUILD_SOURCEVERSIONAUTHOR=bors
2019-11-22T00:10:44.1742230Z BUILD_SOURCEVERSIONMESSAGE=Auto merge of #66282 - Centril:simplify-try, r=oli-obk
2019-11-22T00:10:44.1742473Z CI_JOB_NAME=x86_64-msvc-cargo
2019-11-22T00:10:44.1742526Z COBERTURA_HOME=C:\cobertura-2.1.1
2019-11-22T00:10:44.1742657Z COMMONPROGRAMFILES=C:\Program Files\Common Files
2019-11-22T00:10:44.1742722Z COMMON_TESTRESULTSDIRECTORY=D:\a\1\TestResults
---
2019-11-22T00:10:44.1746232Z HOMEPATH=\Users\VssAdministrator
2019-11-22T00:10:44.1746312Z IEWebDriver=C:\SeleniumWebDrivers\IEDriver
2019-11-22T00:10:44.1746369Z INPUT_ARGUMENTS=
2019-11-22T00:10:44.1748921Z ImageVersion=20191028.1
2019-11-22T00:10:44.1749288Z It should be noted however that the desugaring of `?` includes a function call and so the first pass in this PR relies on inlining to substitute that function call for identity on `x`. Inlining requires `mir-opt-level=2` so this might not have any effect in perf-bot but let's find out.
2019-11-22T00:10:44.1749792Z JAVA_HOME_11_X64=C:\Program Files\Java\zulu-11-azure-jdk_11.33.15-11.0.4-win_x64
2019-11-22T00:10:44.1749876Z JAVA_HOME_7_X64=C:\Program Files\Java\zulu-7-azure-jdk_7.31.0.5-7.0.232-win_x64
2019-11-22T00:10:44.1749972Z JAVA_HOME_8_X64=C:\Program Files\Java\zulu-8-azure-jdk_8.40.0.25-8.0.222-win_x64
2019-11-22T00:10:44.1750060Z LOCALAPPDATA=C:\Users\VssAdministrator\AppData\Local
---
2019-11-22T00:10:44.1790457Z TMP=/tmp
2019-11-22T00:10:44.1790526Z TOOLSTATE_ISSUES_API_URL=https://api.github.com/repos/rust-lang/rust/issues
2019-11-22T00:10:44.1790601Z TOOLSTATE_PUBLISH=1
2019-11-22T00:10:44.1790664Z TOOLSTATE_REPO=https://github.com/rust-lang-nursery/rust-toolstate
2019-11-22T00:10:44.1790759Z This PR works towards https://github.com/rust-lang/rust/issues/66234 by providing two optimization passes meant to run in sequence:
2019-11-22T00:10:44.1790845Z Together, these are meant to simplify the following into just `res`:
2019-11-22T00:10:44.1791153Z USERDOMAIN=fv-az665
2019-11-22T00:10:44.1791225Z USERDOMAIN_ROAMINGPROFILE=fv-az665
2019-11-22T00:10:44.1791297Z USERNAME=VssAdministrator
2019-11-22T00:10:44.1791353Z USERPROFILE=C:\Users\VssAdministrator
2019-11-22T00:10:44.1791353Z USERPROFILE=C:\Users\VssAdministrator
2019-11-22T00:10:44.1791431Z VCPKG_INSTALLATION_ROOT=C:\vcpkg
2019-11-22T00:10:44.1791487Z VCVARS_BAT=vcvars64.bat
2019-11-22T00:10:44.1791566Z VS140COMNTOOLS=C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\Tools\
2019-11-22T00:10:44.1791634Z VSTS_AGENT_PERFLOG=c:\vsts\perflog
2019-11-22T00:10:44.1791717Z VSTS_PROCESS_LOOKUP_ID=vsts_2d5c59f1-62fe-4ead-a819-c4591a3bcde2
2019-11-22T00:10:44.1791794Z WINDIR=C:\windows
2019-11-22T00:10:44.1791852Z WIX=C:\Program Files (x86)\WiX Toolset v3.11\
2019-11-22T00:10:44.1791934Z [mir-opt] asking `?`s in a more optimized fashion
2019-11-22T00:10:44.1792059Z ```
2019-11-22T00:10:44.1792104Z ```rust
2019-11-22T00:10:44.1792166Z agent.jobstatus=Succeeded
2019-11-22T00:10:44.1792219Z match res {
2019-11-22T00:10:44.1792219Z match res {
2019-11-22T00:10:44.1792301Z r? @oli-obk -- This is WIP, but I'd appreciate feedback. :)
2019-11-22T00:10:44.1792438Z 
2019-11-22T00:10:44.1796114Z disk usage:
2019-11-22T00:10:44.2281915Z Filesystem            Size  Used Avail Use% Mounted on
2019-11-22T00:10:44.2282059Z C:/Program Files/Git  256G  140G  116G  55% /
---
2019-11-22T00:10:55.3411300Z  30  480M   30  146M    0     0  22.1M      0  0:00:21  0:00:06  0:00:15 22.3M
2019-11-22T00:10:56.7898963Z  36  480M   36  173M    0     0  22.7M      0  0:00:21  0:00:07  0:00:14 23.3M
2019-11-22T00:10:56.8221928Z  38  480M   38  186M    0     0  20.6M      0  0:00:23  0:00:09  0:00:14 20.1M
2019-11-22T00:10:56.8249797Z  39  480M   39  187M    0     0  20.6M      0  0:00:23  0:00:09  0:00:14 20.0M
2019-11-22T00:10:56.8250879Z curl: (56) OpenSSL SSL_read: SSL_ERROR_SYSCALL, errno 10054
2019-11-22T00:10:56.8271581Z 
2019-11-22T00:10:56.8271869Z gzip: stdin: unexpected end of file
2019-11-22T00:10:56.8277445Z tar: Unexpected EOF in archive
2019-11-22T00:10:56.8277542Z tar: Unexpected EOF in archive
2019-11-22T00:10:56.8277621Z tar: Error is not recoverable: exiting now
2019-11-22T00:10:56.8334979Z 
2019-11-22T00:10:56.8412459Z ##[error]Bash exited with code '2'.
2019-11-22T00:10:56.8597332Z ##[section]Starting: Checkout
2019-11-22T00:10:56.8693690Z ==============================================================================
2019-11-22T00:10:56.8693798Z Task         : Get sources
2019-11-22T00:10:56.8693864Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@bors

@bors bors added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

labels

Nov 22, 2019

@Centril

@bors bors 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 22, 2019

@bors

bors added a commit that referenced this pull request

Nov 22, 2019

@bors

[mir-opt] asking ?s in a more optimized fashion

This PR works towards #66234 by providing two optimization passes meant to run in sequence:

Together, these are meant to simplify the following into just res:

match res {
    Ok(x) => Ok(x),
    Err(x) => Err(x),
}

It should be noted however that the desugaring of ? includes a function call and so the first pass in this PR relies on inlining to substitute that function call for identity on x. Inlining requires mir-opt-level=2 so this might not have any effect in perf-bot but let's find out.

r? @oli-obk -- This is WIP, but I'd appreciate feedback. :)

@bors

@Dylan-DPC-zz

eddyb

| || Some((local_0, vf_s0.var_idx)) != match_set_discr(s2) | | -------------------------------------------------------------- | | { | | continue; | | } |

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're not checking that local_0 != local_1, which is another soundness condition.
cc @rust-lang/wg-mir-opt

eddyb

///
/// ```rust
/// _LOCAL_TMP = ((_LOCAL_1 as Variant ).FIELD: TY );
/// ((_LOCAL_0 as Variant).FIELD: TY) = move _LOCAL_TMP;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only sound to collapse if _LOCAL_TMP isn't used anywhere else (see promote_consts for an example of an analysis like that).

But also, there's another issue here... namely, debuginfo.

You need to replace _LOCAL_TMP with ((_LOCAL_1 as Variant ).FIELD: TY) in debuginfo as well, making this a very specialized form of copy-prop, I guess?

cc @rust-lang/wg-mir-opt

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to preserve debuginfo perfectly, but for that we might need a way to merge scopes or something, so that SourceInfo from all 3 statements is combined.

Effectively, the resulting _LOCAL_0 = move _LOCAL_1 needs to be in a superposition of all the scopes.

Which might be hard due to the inlining.

eddyb

_ => unreachable!(),
}
s1.make_nop();
s2.make_nop();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would replace the third statement and nop the first two. That way, you get the SourceInfo of the Ok(x) rather than the x use (or whatever initialized it).

eddyb

/// Simplifies `SwitchInt(_) -> [targets]`,
/// where all the `targets` have the same form,
/// into `goto -> target_first`.
pub struct SimplifyBranchSame;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can do this unless there is no debuginfo attached to scopes used only in the arms, or we can convert the debuginfo into a form that switches on the SwitchInt operand (plausible in DWARF, not really exposed in LLVM).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But also this optimization can be rephrased as sinking identical statements from predecessors into the start of a block, followed by simplify_cfg.

@eddyb eddyb mentioned this pull request

Feb 8, 2020

JohnTitor added a commit to JohnTitor/rust that referenced this pull request

Feb 14, 2020

@JohnTitor

Labels

merged-by-bors

This PR was explicitly merged by bors.

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.