Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

Add LLVM15-specific codegen test for try/?s that now optimize away #100693

Merged
merged 1 commit into from
Aug 20, 2022
Merged

Add LLVM15-specific codegen test for try/?s that now optimize away #100693

merged 1 commit into from
Aug 20, 2022

Conversation

scottmcm
Copy link
Member

These still generated a bunch of code back in Rust 1.63 (https://rust.godbolt.org/z/z31P8h6rz), but with LLVM 15 merged they no longer do 🎉

@scottmcm
Add LLVM15-specific codegen test for try/?s that now optimize away
5145c97 
These still generated a bunch of code back in Rust 1.63 (<https://rust.godbolt.org/z/z31P8h6rz>), but with LLVM 15 merged they no longer do 🎉
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 17, 2022
This was referenced Aug 17, 2022
Closed
Closed
@scottmcm
Copy link
Member Author

@bors rollup=iffy (codegen test that doesn't run in the PR build, because that uses LLVM 13 but the test needs LLVM 15)

Kobzol
Kobzol reviewed Aug 17, 2022
View reviewed changes
Copy link
Contributor

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Could you also include a test with Ok(x?)? I think that this pattern is also quite common.

@scottmcm
Copy link
Member Author

scottmcm commented Aug 17, 2022 •
edited
Loading

Could you also include a test with Ok(x?)?

Because MIR inlining is on now, that's very much the same as the try { x? } one now -- you can look at the MIR in https://play.rust-lang.org/?version=nightly&mode=release&edition=2021&gist=80afa100459d91316f209cfb2d8100d8 and see the "inlined <Result<i32, u32> as Try>::from_output" and that function is just Ok:

rust/library/core/src/result.rs

Lines 2111 to 2114 in 9c20b2a

#[inline]
fn from_output(output: Self::Output) -> Self {
Ok(output)
}

Basically what I picked here is the direct one -- the obvious match -- and the indirect one -- that to NOP has to see through all the trait calls -- so if the most-indirect one is fine, I'm not sure it's worth also listing the spots in the middle.

@scottmcm scottmcm mentioned this pull request Aug 19, 2022
Merged
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Aug 20, 2022

📌 Commit 5145c97 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@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 Aug 20, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Aug 20, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 20, 2022
@bors
Auto merge of rust-lang#100809 - matthiaskrgr:rollup-xkpd5ii, r=matth…
48853a3 
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#99415 (Initial implementation of REUSE)
 - rust-lang#99544 (Expose `Utf8Lossy` as `Utf8Chunks`)
 - rust-lang#100585 (Fix trailing space showing up in example)
 - rust-lang#100596 (Remove unnecessary stderr files)
 - rust-lang#100642 (Update fortanix-sgx-abi and export some useful SGX usercall traits)
 - rust-lang#100691 (Make `same_type_modulo_infer` a proper `TypeRelation`)
 - rust-lang#100693 (Add LLVM15-specific codegen test for `try`/`?`s that now optimize away)
 - rust-lang#100710 (Windows: Load synch functions together)
 - rust-lang#100807 (Add TaKO8Ki to translation-related mention groups)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 45568bd into rust-lang:master Aug 20, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 20, 2022
@scottmcm scottmcm deleted the new-llvm15-nops branch August 20, 2022 20:10
@nikic nikic mentioned this pull request Aug 30, 2022
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 6, 2022
@bors
Auto merge of rust-lang#100733 - scottmcm:inline-from-from-identity, …
380addd 
…r=m-ou-se

Inline `<T as From<T>>::from`

I noticed (in rust-lang#100693 (comment)) that the MIR for <https://play.rust-lang.org/?version=nightly&mode=release&edition=2021&gist=67097e0494363ee27421a4e3bdfaf513> has inlined most stuff
```
scope 5 (inlined <Result<i32, u32> as Try>::branch)
```
```
scope 8 (inlined <Result<i32, u32> as Try>::from_output)
```

But yet the do-nothing `from` call was still there:
```
_17 = <u32 as From<u32>>::from(move _18) -> bb9;
```

So let's give this a try and see what perf has to say.
workingjubilee pushed a commit to tcdi/postgrestd that referenced this pull request Sep 15, 2022
@bors
Auto merge of #100733 - scottmcm:inline-from-from-identity, r=m-ou-se
c12dc6b 
Inline `<T as From<T>>::from`

I noticed (in rust-lang/rust#100693 (comment)) that the MIR for <https://play.rust-lang.org/?version=nightly&mode=release&edition=2021&gist=67097e0494363ee27421a4e3bdfaf513> has inlined most stuff
```
scope 5 (inlined <Result<i32, u32> as Try>::branch)
```
```
scope 8 (inlined <Result<i32, u32> as Try>::from_output)
```

But yet the do-nothing `from` call was still there:
```
_17 = <u32 as From<u32>>::from(move _18) -> bb9;
```

So let's give this a try and see what perf has to say.
@Stebalien Stebalien mentioned this pull request Jan 2, 2023
Closed
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 20, 2024
@bors
Auto merge of #100733 - scottmcm:inline-from-from-identity, r=m-ou-se
411f3c0 
Inline `<T as From<T>>::from`

I noticed (in rust-lang/rust#100693 (comment)) that the MIR for <https://play.rust-lang.org/?version=nightly&mode=release&edition=2021&gist=67097e0494363ee27421a4e3bdfaf513> has inlined most stuff
```
scope 5 (inlined <Result<i32, u32> as Try>::branch)
```
```
scope 8 (inlined <Result<i32, u32> as Try>::from_output)
```

But yet the do-nothing `from` call was still there:
```
_17 = <u32 as From<u32>>::from(move _18) -> bb9;
```

So let's give this a try and see what perf has to say.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
@bors
Auto merge of #100733 - scottmcm:inline-from-from-identity, r=m-ou-se
d6c0b7b 
Inline `<T as From<T>>::from`

I noticed (in rust-lang/rust#100693 (comment)) that the MIR for <https://play.rust-lang.org/?version=nightly&mode=release&edition=2021&gist=67097e0494363ee27421a4e3bdfaf513> has inlined most stuff
```
scope 5 (inlined <Result<i32, u32> as Try>::branch)
```
```
scope 8 (inlined <Result<i32, u32> as Try>::from_output)
```

But yet the do-nothing `from` call was still there:
```
_17 = <u32 as From<u32>>::from(move _18) -> bb9;
```

So let's give this a try and see what perf has to say.
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@Kobzol Kobzol Kobzol left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.65.0
Development

Successfully merging this pull request may close these issues.
6 participants
@scottmcm @rust-highfive @Mark-Simulacrum @bors @Kobzol @rustbot