GeezerVison

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label

Centril commented Nov 11, 2019

Copy link

Copy Markdown

Contributor Author

@bors try @rust-timer queue

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rust-timer commented Nov 11, 2019

Copy link

Copy Markdown

Collaborator

Awaiting bors try build completion

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bors commented Nov 11, 2019

Copy link

Copy Markdown

Collaborator

⌛ Trying commit 91e5a0e with merge fb1a35ea56157ef7f76c1451f72cda0b3be86a38...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Centril commented Nov 11, 2019

Copy link

Copy Markdown

Contributor Author

@bors retry

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Centril commented Nov 11, 2019

Copy link

Copy Markdown

Contributor Author

@bors retry try

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bors commented Nov 11, 2019

Copy link

Copy Markdown

Collaborator

⌛ Trying commit 657c945 with merge 0113342...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bors added a commit that referenced this pull request


          Auto merge of #66282 - Centril:simplify-try, r=<try>

[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:

- `SimplifyArmIdentity` which transforms something like:
  ```rust
  _LOCAL_TMP = ((_LOCAL_1 as Variant ).FIELD: TY );
  ((_LOCAL_0 as Variant).FIELD: TY) = move _LOCAL_TMP;
  discriminant(_LOCAL_0) = VAR_IDX;
  ```

  into:

  ```rust
  _LOCAL_0 = move _LOCAL_1
  ```

- `SimplifyBranchSame` which transforms `SwitchInt`s to identical basic blocks into a `goto` to the first reachable target.

Together, these are meant to simplify the following into just `res`:
```rust
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 commented

View reviewed changes

Comment thread

src/test/mir-opt/simplify_try.rs

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

Centril Nov 11, 2019

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

oli-obk Nov 11, 2019

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

wesleywiser Nov 11, 2019

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

oli-obk Nov 12, 2019

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

wesleywiser reacted with thumbs up emoji

All reactions

👍 1 reaction

Centril Nov 12, 2019

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

oli-obk Nov 12, 2019

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Centril commented

View reviewed changes

Comment thread

src/librustc_mir/transform/simplify_try.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Centril commented

View reviewed changes

Comment thread

src/librustc_mir/transform/mod.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Centril commented

View reviewed changes

Comment thread

src/librustc_mir/transform/simplify_try.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

This comment has been minimized.

bors commented Nov 11, 2019

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rust-timer commented Nov 11, 2019

Copy link

Copy Markdown

Collaborator

Queued 0113342 with parent e2fa952, future comparison URL.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

oli-obk reviewed

View reviewed changes

Comment thread

src/librustc_mir/transform/simplify_try.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

rust-timer commented Nov 11, 2019

Copy link

Copy Markdown

Collaborator

Finished benchmarking try commit 0113342, comparison URL.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This comment has been minimized.

Centril force-pushed the simplify-try branch from 219d24f to f026031 Compare

November 12, 2019 12:38

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 commented Nov 12, 2019

Copy link

Copy Markdown

Contributor Author

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:

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.
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).

wesleywiser reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Centril commented Nov 12, 2019 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 commented Nov 12, 2019

Copy link

Copy Markdown

Contributor Author

This should be ready now :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 commented Nov 22, 2019

Copy link

Copy Markdown

Collaborator

⌛ Testing commit 2f00e86 with merge ae731e69b0e519dbf73d4a870a82224b778808de...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rust-highfive commented Nov 22, 2019

Copy link

Copy Markdown

Contributor

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)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bors commented Nov 22, 2019

Copy link

Copy Markdown

Collaborator

💔 Test failed - checks-azure

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Centril commented Nov 22, 2019

Copy link

Copy Markdown

Contributor Author

@bors retry spurious

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bors commented Nov 22, 2019

Copy link

Copy Markdown

Collaborator

⌛ Testing commit 2f00e86 with merge abd6955...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bors added a commit that referenced this pull request


          Auto merge of #66282 - Centril:simplify-try, r=oli-obk

abd6955

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

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

- `SimplifyArmIdentity` which transforms something like:
  ```rust
  _LOCAL_TMP = ((_LOCAL_1 as Variant ).FIELD: TY );
  ((_LOCAL_0 as Variant).FIELD: TY) = move _LOCAL_TMP;
  discriminant(_LOCAL_0) = VAR_IDX;
  ```

  into:

  ```rust
  _LOCAL_0 = move _LOCAL_1
  ```

- `SimplifyBranchSame` which transforms `SwitchInt`s to identical basic blocks into a `goto` to the first reachable target.

Together, these are meant to simplify the following into just `res`:
```rust
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 commented Nov 22, 2019

Copy link

Copy Markdown

Collaborator

☀️ Test successful - checks-azure
Approved by: oli-obk
Pushing abd6955 to master...

tesuji, Aaron1011, and mark-i-m reacted with hooray emoji

All reactions

🎉 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bors added the merged-by-bors This PR was explicitly merged by bors. label

bors merged commit 2f00e86 into rust-lang:master

[MIR-opt] trivial matches aren't optimized out at mir-opt-level=1 #66855

Centril deleted the simplify-try branch

November 22, 2019 05:33

Dylan-DPC-zz commented Nov 28, 2019

Copy link

Copy Markdown

@oli-obk sorry wrong pr 😂

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

scottmcm mentioned this pull request

Nov 28, 2019

Closed

bytwise mentioned this pull request

Nov 28, 2019

ICE with mir-opt-level=2: store_with_flags: invalid ScalarPair layout #66856

Closed

horse-princess mentioned this pull request

Nov 29, 2019

Miscompilation with recent Nightlies #66851

Closed

Mark-Simulacrum mentioned this pull request

Dec 5, 2019

Optimize out nop-matches #66234

Open

eddyb reviewed

View reviewed changes

Comment thread

src/librustc_mir/transform/simplify_try.rs

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

eddyb Feb 8, 2020

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

eddyb reviewed

View reviewed changes

Comment thread

src/librustc_mir/transform/simplify_try.rs

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

eddyb Feb 8, 2020

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

eddyb Feb 8, 2020

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

eddyb reviewed

View reviewed changes

Comment thread

src/librustc_mir/transform/simplify_try.rs

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

eddyb Feb 8, 2020

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

eddyb reviewed

View reviewed changes

Comment thread

src/librustc_mir/transform/simplify_try.rs

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

eddyb Feb 8, 2020

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

eddyb Feb 8, 2020

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

eddyb mentioned this pull request