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

Inline copied and next methods #108048

Closed

Conversation

dianqk
Copy link
Member

@dianqk dianqk commented Feb 14, 2023

This should fix #107681.

@rustbot
Copy link
Collaborator

rustbot commented Feb 14, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @scottmcm (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 14, 2023
@rustbot
Copy link
Collaborator

rustbot commented Feb 14, 2023

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@lukas-code
Copy link
Member

Can you add a codegen test for this?

Something like // CHECK-NOT: br should be enough.

@dianqk dianqk force-pushed the inline-copied-and-next-methods branch from 8118cb3 to b2158c7 Compare February 14, 2023 21:45
@dianqk
Copy link
Member Author

dianqk commented Feb 14, 2023

Can you add a codegen test for this?

Something like // CHECK-NOT: br should be enough.

Thanks for your help. I tried to add the corresponding test.

@scottmcm
Copy link
Member

In the LLVM in the issue the methods are inlined, though? There's no calls...

@dianqk
Copy link
Member Author

dianqk commented Feb 14, 2023

In the LLVM in the issue the methods are inlined, though? There's no calls...

Yes, the methods are inlined. I think it should be an appropriate approach to add an inline attribute early.

@scottmcm
Copy link
Member

Looks like the #[inline]s didn't actually fix it, since the branch is still there in the CI failure https://github.com/rust-lang/rust/actions/runs/4178347433/jobs/7237027819#step:26:1813

@rust-log-analyzer

This comment has been minimized.

@dianqk
Copy link
Member Author

dianqk commented Feb 15, 2023

I noticed that the release of rustc comes bundled with LLVM 15 (for local development as well). Why is LLVM 13 used on CI?

More, I found a similar example to accomplish the relevant optimizations. The structure looks similar to LLVM IR, and it should be possible to solve the problem with some Passes improvements.

https://rust.godbolt.org/z/aoYxdzWsv

pub unsafe fn demo_std_iter(x: &mut Iter<'_, u32>) -> u32 {
    *x.next().unwrap_unchecked()
}

@scottmcm
Copy link
Member

LLVM 13 is used for the PR build only; the full merge checks have runners that use 13, 14, 15, and the bundled one.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 15, 2023
@dianqk dianqk force-pushed the inline-copied-and-next-methods branch from b2158c7 to 4385eb3 Compare February 15, 2023 12:06
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-14 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v3' (SHA:ac593985615ec2ede58e132d2e21d2b1cbd6127c)
Download action repository 'rust-lang/simpleinfra@master' (SHA:7ea263296c702710d7fac3c6d5bccdb2895b4e79)
Complete job name: PR (x86_64-gnu-llvm-14, false, ubuntu-20.04-xl)
git config --global core.autocrlf false
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
  CI_JOB_NAME: x86_64-gnu-llvm-14
---
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

running 404 tests
i......i.i............i....i..ii.................iii........i.i.i........i.............. 88/404
.....ii.................i............i.....i...............i.....i...iiii.......i.Fi.... 176/404
..........ii........................i.i.ii.i.i...............i.....i...i...iii......i... 352/404
.i.....................iiiiiiii.i...................
failures:
Some tests failed in compiletest suite=codegen mode=codegen host=x86_64-unknown-linux-gnu target=x86_64-unknown-linux-gnu
Some tests failed in compiletest suite=codegen mode=codegen host=x86_64-unknown-linux-gnu target=x86_64-unknown-linux-gnu

---- [codegen] tests/codegen/issue-107681-unwrap_unchecked-optimized.rs stdout ----

error: verification with 'FileCheck' failed
status: exit status: 1
command: "/usr/lib/llvm-14/bin/FileCheck" "--input-file" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/issue-107681-unwrap_unchecked-optimized/issue-107681-unwrap_unchecked-optimized.ll" "/checkout/tests/codegen/issue-107681-unwrap_unchecked-optimized.rs" "--allow-unused-prefixes" "--check-prefixes" "CHECK,NONMSVC" "--dump-input-context" "100"
stdout: none
--- stderr -------------------------------
/checkout/tests/codegen/issue-107681-unwrap_unchecked-optimized.rs:11:16: error: CHECK-NOT: excluded string found in input
 // CHECK-NOT: br
               ^
/checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/issue-107681-unwrap_unchecked-optimized/issue-107681-unwrap_unchecked-optimized.ll:23:2: note: found here
 br i1 %_10.i, label %bb4.split, label %bb6.split

Input file: /checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/issue-107681-unwrap_unchecked-optimized/issue-107681-unwrap_unchecked-optimized.ll
Check file: /checkout/tests/codegen/issue-107681-unwrap_unchecked-optimized.rs


-dump-input=help explains the following input dump.
Input was:
<<<<<<
<<<<<<
        1: ; ModuleID = 'issue_107681_unwrap_unchecked_optimized.91c38bd7-cgu.0' 
        2: source_filename = "issue_107681_unwrap_unchecked_optimized.91c38bd7-cgu.0" 
        3: target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128" 
        4: target triple = "x86_64-unknown-linux-gnu" 
        5:  
        6: %"core::panic::location::Location<'_>" = type { { [0 x i8]*, i64 }, i32, i32 } 
        7: %"unwind::libunwind::_Unwind_Exception" = type { i64, void (i32, %"unwind::libunwind::_Unwind_Exception"*)*, [2 x i64] } 
        8: %"unwind::libunwind::_Unwind_Context" = type { [0 x i8] } 
        9:  
       10: @alloc24 = private unnamed_addr constant <{ [32 x i8] }> <{ [32 x i8] c"assertion failed: self.is_some()" }>, align 1 
       11: @alloc25 = private unnamed_addr constant <{ [66 x i8] }> <{ [66 x i8] c"/checkout/tests/codegen/issue-107681-unwrap_unchecked-optimized.rs" }>, align 1 
       12: @alloc26 = private unnamed_addr constant <{ i8*, [16 x i8] }> <{ i8* getelementptr inbounds (<{ [66 x i8] }>, <{ [66 x i8] }>* @alloc25, i32 0, i32 0, i32 0), [16 x i8] c"B\00\00\00\00\00\00\00\0C\00\00\00\0E\00\00\00" }>, align 8 
       14: ; issue_107681_unwrap_unchecked_optimized::unwrap_unchecked_optimized 
       14: ; issue_107681_unwrap_unchecked_optimized::unwrap_unchecked_optimized 
       15: ; Function Attrs: nonlazybind uwtable 
       16: define noundef i32 @_ZN39issue_107681_unwrap_unchecked_optimized26unwrap_unchecked_optimized17h230e308c612ab056E({ i32*, i32* }* noalias nocapture noundef align 8 dereferenceable(16) %x) unnamed_addr #0 personality i32 (i32, i32, i64, %"unwind::libunwind::_Unwind_Exception"*, %"unwind::libunwind::_Unwind_Context"*)* @rust_eh_personality { 
       17: start: 
       18:  %0 = getelementptr inbounds { i32*, i32* }, { i32*, i32* }* %x, i64 0, i32 1 
       19:  %self1.i = load i32*, i32** %0, align 8, !alias.scope !2, !nonnull !5, !noundef !5 
       20:  %1 = getelementptr inbounds { i32*, i32* }, { i32*, i32* }* %x, i64 0, i32 0 
       21:  %self2.i = load i32*, i32** %1, align 8, !alias.scope !2, !nonnull !5, !noundef !5 
       22:  %_10.i = icmp eq i32* %self1.i, %self2.i 
       23:  br i1 %_10.i, label %bb4.split, label %bb6.split 
not:11      !~                                                error: no match expected
       24:  
       25: bb4.split: ; preds = %start 
       26: ; call core::panicking::panic 
       27:  tail call void @_ZN4core9panicking5panic17h1b9927247f064db9E([0 x i8]* noalias noundef nonnull readonly align 1 bitcast (<{ [32 x i8] }>* @alloc24 to [0 x i8]*), i64 noundef 32, %"core::panic::location::Location<'_>"* noalias noundef readonly align 8 dereferenceable(24) bitcast (<{ i8*, [16 x i8] }>* @alloc26 to %"core::panic::location::Location<'_>"*)) #2 
       28:  unreachable 
       29:  
       30: bb6.split: ; preds = %start 
       31:  %2 = getelementptr inbounds i32, i32* %self1.i, i64 1 
       32:  store i32* %2, i32** %0, align 8, !alias.scope !2 
       33:  %v = load i32, i32* %self1.i, align 4, !noundef !5 
       34:  ret i32 %v 
       35: } 
       36:  
       37: ; Function Attrs: nonlazybind uwtable 
       38: declare noundef i32 @rust_eh_personality(i32 noundef, i32 noundef, i64 noundef, %"unwind::libunwind::_Unwind_Exception"* noundef, %"unwind::libunwind::_Unwind_Context"* noundef) unnamed_addr #0 
       40: ; core::panicking::panic 
       40: ; core::panicking::panic 
       41: ; Function Attrs: cold noinline noreturn nonlazybind uwtable 
       42: declare void @_ZN4core9panicking5panic17h1b9927247f064db9E([0 x i8]* noalias noundef nonnull readonly align 1, i64 noundef, %"core::panic::location::Location<'_>"* noalias noundef readonly align 8 dereferenceable(24)) unnamed_addr #1 
       43:  
       44: attributes #0 = { nonlazybind uwtable "probe-stack"="__rust_probestack" "target-cpu"="x86-64" } 
       45: attributes #1 = { cold noinline noreturn nonlazybind uwtable "probe-stack"="__rust_probestack" "target-cpu"="x86-64" } 
       46: attributes #2 = { noreturn } 
       47:  
       48: !llvm.module.flags = !{!0, !1} 
       49:  
       50: !0 = !{i32 7, !"PIC Level", i32 2} 
       51: !1 = !{i32 2, !"RtLibUseGOT", i32 1} 
       52: !2 = !{!3} 
       53: !3 = distinct !{!3, !4, !"_ZN91_$LT$core..slice..iter..Iter$LT$T$GT$$u20$as$u20$core..iter..traits..iterator..Iterator$GT$4next17h93bcd1515c751190E: %self"} 
       54: !4 = distinct !{!4, !"_ZN91_$LT$core..slice..iter..Iter$LT$T$GT$$u20$as$u20$core..iter..traits..iterator..Iterator$GT$4next17h93bcd1515c751190E"} 
       55: !5 = !{} 
------------------------------------------



@dianqk
Copy link
Member Author

dianqk commented Feb 15, 2023

Thanks for the answer. I will close this PR, although adding #[inline] works on LLVM 15, but it looks like this should work out on LLVM.

@dianqk dianqk closed this Feb 15, 2023
@dianqk dianqk deleted the inline-copied-and-next-methods branch August 6, 2023 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.next().unwrap_unchecked() on Iterator doesn't optimize as expected
5 participants