Skip to content

#77856 causes a minor performance regression in stage2 clang builds #81723

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
Closed
dianqk opened this issue Feb 14, 2024 · 3 comments · Fixed by #85160
Closed

#77856 causes a minor performance regression in stage2 clang builds #81723

dianqk opened this issue Feb 14, 2024 · 3 comments · Fixed by #85160
Assignees
@dianqk
Labels
clang Clang issues not falling into any other category performance

Comments

@dianqk
Copy link
Member

dianqk commented Feb 14, 2024 •
edited
Loading

FWIW, it seems like this causes a minor performance regression in stage2 clang builds (https://llvm-compile-time-tracker.com/compare.php?from=5aec9392674572fa5a06283173a6a739742d261d&to=5932fcc47855fdd209784f38820422d2369b84b2&stat=instructions:u). Though code size does reduce as well.

Originally posted by @nikic in #77856 (comment)
@github-actions github-actions bot added the clang Clang issues not falling into any other category label Feb 14, 2024
@dianqk dianqk self-assigned this Feb 14, 2024
This was referenced Mar 13, 2024
Merged
Merged
dianqk added a commit that referenced this issue May 5, 2024
@dianqk
[InlineCost] Correct the default branch cost for the switch statement (…
882814e 
…#85160)

Fixes #81723.

The earliest commit of the related code is:
919f9e8.
I tried to understand the following code with
#77856 (comment).

https://github.com/llvm/llvm-project/blob/5932fcc47855fdd209784f38820422d2369b84b2/llvm/lib/Analysis/InlineCost.cpp#L709-L720

I think only scenarios where there is a default branch were considered.
@dianqk
Copy link
Member Author

dianqk commented May 5, 2024

Reopen, as the compilation time hasn't been restored, and there are some test cases failing simultaneously.

@dianqk dianqk reopened this May 5, 2024
@dianqk
Copy link
Member Author

dianqk commented May 7, 2024

@nikic I can't reproduce the issue by reverting the relevant code based on the current main branch right now.

This is the revert mentioned in the PR: https://llvm-compile-time-tracker.com/compare.php?from=f3c5278efa3b783ada9e7a34b751cf4c5b864535&to=58622ef6755a02f97e5127bea29ed5b8812fe25e&stat=instructions:u. I can see the expected outcome: a reduction in stage2-O3.

I can see similar result before merging the PR: https://llvm-compile-time-tracker.com/compare.php?from=4f88c2311130791cf69da34b743b1b3ba7584a7b&to=98b32349eb3fdda673a415681524ae3e288ecd26&stat=instructions:u.

But I can't see the expected result after merging: https://llvm-compile-time-tracker.com/compare.php?from=de9b386f84b58ad0ffc12e221bc6d9161ca5b62d&to=882814edd33cab853859f07b1dd4c4fa1393e0ea&stat=instructions:u.

I performed a revert based on the new commit and still couldn't achieve the expected result: https://llvm-compile-time-tracker.com/compare.php?from=1241e7692a466ceb420be2780f1c3e8bbab7d469&to=e983d001859e4d4f1c8c0587dec34dc6fa6fad80&stat=instructions:u.

I speculate that among those 1800 commits, the issue was somehow addressed: https://llvm-compile-time-tracker.com/compare.php?from=4f88c2311130791cf69da34b743b1b3ba7584a7b&to=de9b386f84b58ad0ffc12e221bc6d9161ca5b62d&stat=instructions%3Au. I should be able to close this issue now.

Based on the improvements mentioned by @dtcxzyw on #85160 (comment), I believe I only need to address the test failures on the CI and reland it.

@nikic
Copy link
Contributor

nikic commented May 8, 2024

@dianqk Thanks for looking into it! Agree that we can go ahead and close this issue.

@nikic nikic closed this as completed May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dianqk dianqk

Labels
clang Clang issues not falling into any other category performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[InlineCost] Correct the default branch cost for the switch statement dianqk/llvm-project
3 participants
@nikic @EugeneZelenko @dianqk