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.

Sign up for GitHub

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

Jump to bottom

LLVM doesn't understand Some(Box<u32>) is always some #36010

Closed
alexcrichton opened this issue Aug 26, 2016 · 8 comments · Fixed by #125347
Closed

LLVM doesn't understand Some(Box<u32>) is always some #36010

alexcrichton opened this issue Aug 26, 2016 · 8 comments · Fixed by #125347
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

alexcrichton commented Aug 26, 2016 •
edited
Loading

playground

For the following code:

use std::mem;

fn foo<T>(a: &mut T, b: T) -> bool {
    let b = Some(mem::replace(a, b));
    let ret = b.is_some();
    mem::forget(b);
    return ret
}

pub fn foo_u32(a: &mut u32, b: u32) -> bool {
    foo(a, b)
}

pub fn foo_box(a: &mut Box<u32>, b: Box<u32>) -> bool {
    foo(a, b)
}

This compiles down to the IR (optimized)

; Function Attrs: norecurse uwtable
define zeroext i1 @_ZN3foo7foo_u3217h93ddaaeb1c2cc2b2E(i32* nocapture dereferenceable(4), i32) unnamed_addr #0  {
entry-block:
  store i32 %1, i32* %0, align 4
  ret i1 true
}

; Function Attrs: norecurse uwtable
define zeroext i1 @_ZN3foo7foo_box17hd05578023690d1faE(i32** nocapture dereferenceable(8), i32* noalias dereferenceable(4)) unnamed_addr #0  {
entry-block:
  %2 = ptrtoint i32* %1 to i64
  %3 = bitcast i32** %0 to i64*
  %4 = bitcast i32** %0 to i8**
  %5 = load i8*, i8** %4, align 8, !noalias !0
  store i64 %2, i64* %3, align 8, !noalias !0
  %not.switchtmp.i.i = icmp ne i8* %5, null
  ret i1 %not.switchtmp.i.i
}

attributes #0 = { norecurse uwtable }
attributes #1 = { nounwind }

It seems odd that the Box case is not as optimized as the u32 case? This seems due to the null-pointer optimization of the Option<Box> itself. I wonder if there's more metadata we can attach to tell LLVM the loads aren't null?
@eddyb
Copy link
Member

eddyb commented Aug 26, 2016 •
edited
Loading

Probably using !nonnull on loads would work.

The fundamental problem is that it's hard to decide when to apply metadata because of how completely arbitrary and unreasonably difficult our ADT handling is (i.e. trans::adt).
Thankfully, that can now be eroded away, and once we have a bit more uniformity we can start doing things better.

That said, we already keep that information so it could be used... oh, huh, we already apply !nonnull.
This is probably a more complex interaction between variant accesses and optimizations then.

@bluss
Copy link
Member

bluss commented Aug 26, 2016

Same for Some(&T), affects a lot of iterator optimizations.

@Mark-Simulacrum Mark-Simulacrum added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. I-slow Issue: Problems and improvements with respect to performance of generated code. labels Jun 23, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 26, 2017
@nikic
Copy link
Contributor

nikic commented Nov 3, 2018

I was wondering why this optimizes for me locally, but not on playground ... turns out it optimizes properly at O2, but not at O3 :(

@nikic
Copy link
Contributor

nikic commented Nov 3, 2018

The cause is argument promotion. A pointer argument is promoted to a scalar, and looses the !nonnull metadata in the process.

@bugadani
Copy link
Contributor

bugadani commented Sep 20, 2020 •
edited
Loading

Since rustc 1.45 the following IR is emitted at --opt-level=3:

define zeroext i1 @_ZN7example7foo_box17hd63365026be7dd71E(i32** nocapture align 8 dereferenceable(8) %a, i32* noalias nonnull align 4 %b) unnamed_addr #0 personality i32 (i32, i32, i64, %"unwind::libunwind::_Unwind_Exception"*, %"unwind::libunwind::_Unwind_Context"*)* @rust_eh_personality !dbg !29 {
  %0 = ptrtoint i32* %b to i64
  %1 = bitcast i32** %a to i64*, !dbg !30
  store i64 %0, i64* %1, align 8, !dbg !43, !noalias !45
  ret i1 true, !dbg !50
}

@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
@workingjubilee workingjubilee added the C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such label Oct 8, 2023
@DianQK
Copy link
Member

DianQK commented Mar 28, 2024 •
edited
Loading

rustc 1.65 has already fixed this issue: https://rust.godbolt.org/z/8cz6rn3v1.

@rustbot label E-needs-test -llvm-fixed-upstream (I used the wrong label.)

@rustbot rustbot added E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. llvm-fixed-upstream Issue expected to be fixed by the next major LLVM upgrade, or backported fixes labels Mar 28, 2024
@rustbot
Copy link
Collaborator

rustbot commented Mar 29, 2024

Error: Parsing relabel command in comment failed: ...'d-upstream' | error: a label delta at >| ' (I used t'...

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@DianQK
Copy link
Member

DianQK commented Mar 29, 2024

@rustbot label -llvm-fixed-upstream

@rustbot rustbot removed the llvm-fixed-upstream Issue expected to be fixed by the next major LLVM upgrade, or backported fixes label Mar 29, 2024
tesuji added a commit to tesuji/rustc that referenced this issue May 20, 2024
@tesuji
add codegen test for rust-lang#36010
f6420f2
@tesuji tesuji mentioned this issue May 20, 2024
Merged
tesuji added a commit to tesuji/rustc that referenced this issue May 20, 2024
@tesuji
add codegen test for rust-lang#36010
80d102a
tesuji added a commit to tesuji/rustc that referenced this issue Jun 8, 2024
@tesuji
add codegen test for rust-lang#36010
1e0d63b
tesuji added a commit to tesuji/rustc that referenced this issue Jun 9, 2024
@tesuji
add codegen test for rust-lang#36010
95740fb
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 9, 2024
@bors
Auto merge of rust-lang#125347 - tesuji:needtests, r=nikic
b7edafe 
Add codegen tests for E-needs-test

close rust-lang#36010
close rust-lang#68667
close rust-lang#74938
close rust-lang#83585
close rust-lang#93036
close rust-lang#109328
close rust-lang#110797
close rust-lang#111508
close rust-lang#112509
close rust-lang#113757
close rust-lang#120440
close rust-lang#118392
close rust-lang#71096

r? nikic
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 10, 2024
@bors
Auto merge of rust-lang#125347 - tesuji:needtests, r=nikic
a1b4415 
Add codegen tests for E-needs-test

close rust-lang#36010
close rust-lang#68667
close rust-lang#74938
close rust-lang#83585
close rust-lang#93036
close rust-lang#109328
close rust-lang#110797
close rust-lang#111508
close rust-lang#112509
close rust-lang#113757
close rust-lang#120440
close rust-lang#118392
close rust-lang#71096

r? nikic
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 11, 2024
@bors
Auto merge of rust-lang#125347 - tesuji:needtests, r=nikic
b1761a2 
Add codegen tests for E-needs-test

close rust-lang#36010
close rust-lang#68667
close rust-lang#74938
close rust-lang#83585
close rust-lang#93036
close rust-lang#109328
close rust-lang#110797
close rust-lang#111508
close rust-lang#112509
close rust-lang#113757
close rust-lang#120440
close rust-lang#118392
close rust-lang#71096

r? nikic
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 13, 2024
@bors
Auto merge of rust-lang#125347 - tesuji:needtests, r=nikic
4936869 
Add codegen tests for E-needs-test

close rust-lang#36010
close rust-lang#68667
close rust-lang#74938
close rust-lang#83585
close rust-lang#93036
close rust-lang#109328
close rust-lang#110797
close rust-lang#111508
close rust-lang#112509
close rust-lang#113757
close rust-lang#120440
close rust-lang#118392
close rust-lang#71096

r? nikic
@bors bors closed this as completed in 7ac6c2f Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add codegen tests for E-needs-test tesuji/rustc
10 participants
@alexcrichton @eddyb @nikic @bugadani @bluss @Mark-Simulacrum @DianQK @workingjubilee @rustbot @Noratrieb