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

[msan] Better handling of signed compare #111212

Closed
eugenis opened this issue Oct 4, 2024 · 4 comments · Fixed by #113200
Closed

[msan] Better handling of signed compare #111212

eugenis opened this issue Oct 4, 2024 · 4 comments · Fixed by #113200
Assignees
@vitalybuka
Labels
llvm:instcombine

Comments

@eugenis
Copy link
Contributor

eugenis commented Oct 4, 2024

#110880 triggers a false positive in MSan.

Improve handleSignedRelationalComparison to handle the kind of IR generated with that change; or enable exact handling (-msan-handle-icmp-exact) at the cost of ~3% code size and (unknown) CPU.
@eugenis eugenis mentioned this issue Oct 4, 2024
Closed
@vitalybuka vitalybuka self-assigned this Oct 4, 2024
@goldsteinn
Copy link
Contributor

Do you still want me to revert a646436? Happy to do so.

@vitalybuka
Copy link
Collaborator

Do you still want me to revert a646436? Happy to do so.

Yes please! It will take some time to optimize -msan-handle-icmp-exact.

Please mention this issue in revert, so we can reland when msan is fixed.

@vitalybuka vitalybuka mentioned this issue Oct 5, 2024
Merged
vitalybuka added a commit that referenced this issue Oct 5, 2024
@vitalybuka
Revert "[InstCombine] Folding (icmp eq/ne (and X, -P2), INT_MIN)" (#…
574266c 
…111236)

Reverts #110880 because of exposed issue is Msan instrumentation
#111212.

This reverts commit a646436.
Kyvangka1610 pushed a commit to Kyvangka1610/llvm-project that referenced this issue Oct 5, 2024
@vitalybuka @Kyvangka1610
Revert "[InstCombine] Folding (icmp eq/ne (and X, -P2), INT_MIN)" (l…
fd576fd 
…lvm#111236)

Reverts llvm#110880 because of exposed issue is Msan instrumentation
llvm#111212.

This reverts commit a646436.
@goldsteinn
Copy link
Contributor

Do you still want me to revert a646436? Happy to do so.

Yes please! It will take some time to optimize -msan-handle-icmp-exact.

Please mention this issue in revert, so we can reland when msan is fixed.

see you did so, thank you. Can you ping back on the PR when recommit is okay?

@vitalybuka vitalybuka mentioned this issue Oct 21, 2024
Merged
vitalybuka added a commit that referenced this issue Oct 22, 2024
@vitalybuka
[msan] Switch to -msan-handle-icmp-exact my default (#113200)
395093e 
Fixes #111212.

This grows .text by 5.3% on CTMark, (or 2.6% large internal binary)
Perf regressed by 1.6%. We will try to improve in follow up patches.

It worth to pay some performance regression to fix
correctness to avoid stuff like #111212.
@vitalybuka
Copy link
Collaborator

vitalybuka commented Oct 22, 2024 •
edited
Loading

@goldsteinn Let see if this one is not reverted and try to reland #110880 tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vitalybuka vitalybuka

Labels
llvm:instcombine
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[msan] Switch to -msan-handle-icmp-exact my default llvm/llvm-project
4 participants
@eugenis @vitalybuka @EugeneZelenko @goldsteinn