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

same functions are not merged in library #91490

Closed
hellow554 opened this issue Dec 3, 2021 · 7 comments
Closed

same functions are not merged in library #91490

hellow554 opened this issue Dec 3, 2021 · 7 comments
Assignees
Labels
C-bug Category: This is a bug. I-heavy Issue: Problems and improvements with respect to binary size of generated code. P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly.

Comments

@hellow554
Copy link
Contributor

I tried this code:

pub fn a(c: char) -> bool {
    c == 's' || c == 'm' || c == 'h' || c == 'd' || c == 'w'
}

pub fn b(c: char) -> bool {
    matches!(c, 's' | 'm' | 'h' | 'd' | 'w')
}

When compiled with the latest stable version and look at the objdump, I see a combined version of a and b, because they are optimized to the same instructions and then merged in the library.
On nightly (already bisected) two copies are stored and it takes the double amount of space (3.9kIB vs 4.1kIB).

Here's a godbolt link: https://rust.godbolt.org/z/zWovojdEE

Version it worked on

It most recently worked on: 1.57.0
Doesn't work since: 63cc2bb cc #88243 @nikic

@rustbot modify labels: +regression-from-stable-to-nightly +I-heavy

@hellow554 hellow554 added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Dec 3, 2021
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. I-heavy Issue: Problems and improvements with respect to binary size of generated code. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Dec 3, 2021
@camelid camelid removed the regression-untriaged Untriaged performance or correctness regression. label Dec 3, 2021
@camelid
Copy link
Member

camelid commented Dec 3, 2021

It most recently worked on: 1.57.0
Doesn't work since: 63cc2bb cc #88243 @nikic

I'm confused: #88243 landed in 1.57.0.

@camelid camelid added regression-untriaged Untriaged performance or correctness regression. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Dec 3, 2021
@camelid
Copy link
Member

camelid commented Dec 3, 2021

Marking as regression-untriaged for now until we figure out which version this actually affects. That PR is in the latest stable (1.57.0), not nightly.

@nikic
Copy link
Contributor

nikic commented Dec 3, 2021

@camelid The change was reverted for the release.

The issue sounds like MergeFuncs doesn't run -- looking at https://github.com/llvm/llvm-project/blob/f178a05f220403f2a9d73c7640bfcc7dc2d7be72/llvm/lib/Passes/PassBuilderPipelines.cpp#L1194 I seem to have misscheduled the pass due to some unfortunate code structure: The passes are added in the middle of OptimizePM construction, but the OptimizePM is only actually scheduled afterwards, which means that MergeFuncs will run close to the start of the module pipeline, not the end. The same seems to have happened to IR outlining and hot-cold splitting.

@camelid camelid added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. and removed regression-untriaged Untriaged performance or correctness regression. labels Dec 3, 2021
@nikic nikic self-assigned this Dec 3, 2021
@nikic
Copy link
Contributor

nikic commented Dec 4, 2021

Test: llvm/llvm-project@5b94037
Fix: https://reviews.llvm.org/D115098, llvm/llvm-project@ae7f468

@apiraino
Copy link
Contributor

apiraino commented Dec 9, 2021

Assigning priority as discussed in the Zulip thread of the Prioritization Working Group.

@rustbot label -I-prioritize +P-medium

@rustbot rustbot added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Dec 9, 2021
nikic added a commit to nikic/rust that referenced this issue Dec 9, 2021
@MSxDOS
Copy link

MSxDOS commented Jan 2, 2022

This is fixed on nightly, apparently by #91657

@camelid
Copy link
Member

camelid commented Jan 2, 2022

Ok, closing then. That PR also added a test for this issue.

@camelid camelid closed this as completed Jan 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. I-heavy Issue: Problems and improvements with respect to binary size of generated code. P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly.
Projects
None yet
Development

No branches or pull requests

6 participants