Skip to content

When we switch to opaque pointers, stop identifying ADTs in LLVM IR (with fewer_names) #96242

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

Closed
erikdesjardins opened this issue Apr 20, 2022 · 0 comments · Fixed by #114350
Closed
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@erikdesjardins
Copy link
Contributor

erikdesjardins commented Apr 20, 2022

This is an extension of #94107. It may be a minor perf win.

Basically, when we switch to using opaque pointers, this comment will no longer be true:

// Use identified structure types for ADT. Due to pointee types in LLVM IR their definition
// might be recursive. Other cases are non-recursive and we can use literal structure types.
ty::Adt(..) => Some(String::new()),

@rustbot label A-llvm T-compiler S-blocked


Edit: the simplest implementation, e.g. changing those lines to

        // In LLVM < 15, use identified structure types for ADT. Due to pointee types in LLVM IR their definition
        // might be recursive. Other cases are non-recursive and we can use literal structure types.
        // In LLVM 15, we use opaque pointers, so there are no pointee types and no potential recursion.
        ty::Adt(..) if get_version() < (15, 0, 0) => Some(String::new()),

doesn't work because then we end up infinitely recursing into the pointee type in order to generate a literal struct type. (Removing pointee types from the codegen backend would avoid this, which I am working on.)

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 20, 2022
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 1, 2023
cleanup: remove pointee types

This can't be merged until the oldest LLVM version we support uses opaque pointers, which will be the case after rust-lang#114148. (Also note `-Cllvm-args="-opaque-pointers=0"` can technically be used in LLVM 15, though I don't think we should support that configuration.)

I initially hoped this would provide some minor perf win, but in rust-lang#105412 (comment) it had very little impact, so this is only valuable as a cleanup.

As a followup, this will enable rust-lang#96242 to be resolved.

r? `@ghost`

`@rustbot` label S-blocked
@bors bors closed this as completed in 73dc6f0 Aug 4, 2023
antoyo pushed a commit to antoyo/rust that referenced this issue Oct 9, 2023
cleanup: remove pointee types

This can't be merged until the oldest LLVM version we support uses opaque pointers, which will be the case after rust-lang#114148. (Also note `-Cllvm-args="-opaque-pointers=0"` can technically be used in LLVM 15, though I don't think we should support that configuration.)

I initially hoped this would provide some minor perf win, but in rust-lang#105412 (comment) it had very little impact, so this is only valuable as a cleanup.

As a followup, this will enable rust-lang#96242 to be resolved.

r? `@ghost`

`@rustbot` label S-blocked
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants