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

replace type_use with LLVM's mergefunc pass #9222

Closed
thestinger opened this issue Sep 16, 2013 · 4 comments · Fixed by #9538
Closed

replace type_use with LLVM's mergefunc pass #9222

thestinger opened this issue Sep 16, 2013 · 4 comments · Fixed by #9538
Labels
A-codegen Area: Code generation C-cleanup Category: PRs that clean code up or issues documenting cleanup.

Comments

@thestinger
Copy link
Contributor

The type_use implementation is buggy, leading to nightmarish code generation errors. The mergefunc pass will be strictly better in terms of code size reduction since it compares equality of the function bytecode after normalizing it. I think it will catch nearly every valid case we currently do with type_use, if not all.

We should drop the maintenance burden as soon as we get mergefunc working (or sooner) and revisit it as a potential compile-time optimization in the future.
@catamorphism catamorphism mentioned this issue Sep 16, 2013
Closed
@thestinger
Copy link
Contributor Author

We also have to figure out where exactly to run mergefunc in the list of passes and #8957 will determine whether it leaves behind stubs.

If we run it early, we'll save on compile-time, and if we run it after optimizations we'll save more on code size. I'm sure how much mergefunc itself is going to cost yet. The big decision is whether to run it before or after the function passes. Of course, this could also vary based on optimization level.

@huonw
Copy link
Member

huonw commented Sep 16, 2013

Can we run it twice?

@thestinger
Copy link
Contributor Author

Yes, but it might be too expensive.

@thestinger
Copy link
Contributor Author

mergefunc is receiving attention upstream, so it will likely be robust enough to enable soon.

It now works well enough for libextra, but not libstd. Sadly, bugpoint fails to narrow down a test case as it ends up chasing down another bug instead...

This was referenced Sep 26, 2013
Closed
Merged
bors added a commit that referenced this issue Sep 27, 2013
@bors
auto merge of #9538 : thestinger/rust/type_use, r=pcwalton
ae8a2ff 
This is broken, and results in poor performance due to the undefined
behaviour in the LLVM IR. LLVM's `mergefunc` is a *much* better way of
doing this since it merges based on the equality of the bytecode.

For example, consider `std::repr`. It generates different code per
type, but is not included in the type bounds of generics.

The `mergefunc` pass works for most of our code but currently hits an
assert on libstd. It is receiving attention upstream so it will be
ready soon, but I don't think removing this broken code should wait any
longer. I've opened #9536 about enabling it by default.

Closes #8651
Closes #3547
Closes #2537
Closes #6971
Closes #9222
flip1995 pushed a commit to flip1995/rust that referenced this issue Aug 11, 2022
@bors
Auto merge of rust-lang#9223 - sgued:unwrap-expect-used, r=giraffate
a591e72 
unwrap_used: Don't recommend using `expect` when the `expect_used` lint is not allowed

Fixes rust-lang#9222

```
changelog: [`unwrap_used`]: Don't recommend using `expect` when the `expect_used` lint is not allowed
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-codegen Area: Code generation C-cleanup Category: PRs that clean code up or issues documenting cleanup.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

remove type_use
2 participants
@huonw @thestinger