Prevent incorrect constant folding #98561
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsFixes #98493
|
src/coreclr/jit/valuenum.cpp
Outdated
| case GT_GT: | ||
| case GT_GE: | ||
| case GT_LT: | ||
| case GT_LE: | ||
| if (IsVNHandle(arg0VN) || IsVNHandle(arg1VN)) |
There was a problem hiding this comment.
Is this going to miss comparisons against 0/null
There was a problem hiding this comment.
Ah, that's a valid point. Do we need to handle it for GT/GE/LT/LE or only EQ/NE?
There was a problem hiding this comment.
@EgorBo may have a better idea. I'd expect we see it for all cases from time to time
There was a problem hiding this comment.
It should be easy enough to fix. I'll push it once I verify it doesn't break on the simple tests.
There was a problem hiding this comment.
It's a bit ugly to read... perhaps a helper IsVNHandleOrNull would be in order?
There was a problem hiding this comment.
Can we just conservatively put these checks into the beginning of VNEvalCanFoldBinaryFunc ? to only allow handles in pair with nullptr. Then we can check the diffs and decide whether it's worth it to relax it.
There was a problem hiding this comment.
Although, Comparisons are fine when both args are handles. E.g. when we compare two frozen objects. So we probably want to avoid only OperIsArithmetic
src/coreclr/jit/valuenum.cpp
Outdated
| @@ -4421,13 +4421,30 @@ bool ValueNumStore::VNEvalCanFoldBinaryFunc(var_types type, VNFunc func, ValueNu | |||
| case GT_RSZ: | |||
| case GT_ROL: | |||
| case GT_ROR: | |||
| if (IsVNHandle(arg0VN) || IsVNHandle(arg1VN)) | |||
There was a problem hiding this comment.
It's unnecessarily conservative to give up on folding handles in the Jit case. I think this should rather refactor GenTreeIntConCommon::ImmedValCanBeFolded a little so that it is callable from VN.
There was a problem hiding this comment.
Sounds reasonable but I am not comfortable enough with the code to do it myself.
Diff results for #98561Assembly diffsAssembly diffs for osx/arm64 ran on linux/x64Diffs are based on 2,293,437 contexts (933,876 MinOpts, 1,359,561 FullOpts). MISSED contexts: base: 0 (0.00%), diff: 6 (0.00%) Overall (-193,772 bytes)
FullOpts (-193,772 bytes)
Details here Throughput diffsThroughput diffs for linux/arm64 ran on windows/x64Overall (+0.03% to +0.05%)
MinOpts (-0.01% to +0.00%)
FullOpts (+0.04% to +0.06%)
Throughput diffs for linux/x64 ran on windows/x64Overall (+0.03% to +0.05%)
FullOpts (+0.04% to +0.08%)
Throughput diffs for osx/arm64 ran on windows/x64Overall (+0.03% to +0.05%)
MinOpts (-0.01% to +0.00%)
FullOpts (+0.04% to +0.06%)
Throughput diffs for windows/arm64 ran on windows/x64Overall (+0.03% to +0.05%)
FullOpts (+0.04% to +0.06%)
Throughput diffs for windows/x64 ran on windows/x64Overall (+0.04% to +0.05%)
FullOpts (+0.04% to +0.08%)
Details here Throughput diffs for linux/arm64 ran on linux/x64Overall (+0.01% to +0.02%)
FullOpts (+0.01% to +0.03%)
Throughput diffs for linux/x64 ran on linux/x64Overall (+0.01% to +0.03%)
FullOpts (+0.01% to +0.05%)
Details here |
Diff results for #98561Assembly diffsAssembly diffs for linux/arm64 ran on windows/x64Diffs are based on 2,544,344 contexts (1,012,496 MinOpts, 1,531,848 FullOpts). MISSED contexts: base: 0 (0.00%), diff: 6 (0.00%) Overall (-209,660 bytes)
FullOpts (-209,660 bytes)
Assembly diffs for linux/x64 ran on windows/x64Diffs are based on 2,535,365 contexts (984,668 MinOpts, 1,550,697 FullOpts). MISSED contexts: base: 0 (0.00%), diff: 6 (0.00%) Overall (+49,121 bytes)
FullOpts (+49,121 bytes)
Assembly diffs for windows/arm64 ran on windows/x64Diffs are based on 2,376,925 contexts (945,150 MinOpts, 1,431,775 FullOpts). MISSED contexts: base: 5 (0.00%), diff: 11 (0.00%) Overall (-199,184 bytes)
FullOpts (-199,184 bytes)
Assembly diffs for windows/x64 ran on windows/x64Diffs are based on 2,416,970 contexts (937,071 MinOpts, 1,479,899 FullOpts). MISSED contexts: base: 0 (0.00%), diff: 6 (0.00%) Overall (+43,548 bytes)
FullOpts (+43,548 bytes)
Details here Assembly diffs for linux/arm ran on windows/x86Diffs are based on 2,250,511 contexts (832,197 MinOpts, 1,418,314 FullOpts). MISSED contexts: base: 73,582 (3.17%), diff: 73,583 (3.17%) Overall (-109,592 bytes)
FullOpts (-109,592 bytes)
Assembly diffs for windows/x86 ran on windows/x86Diffs are based on 2,354,243 contexts (851,840 MinOpts, 1,502,403 FullOpts). MISSED contexts: base: 0 (0.00%), diff: 9 (0.00%) Overall (+3,263 bytes)
FullOpts (+3,263 bytes)
Details here Assembly diffs for osx/arm64 ran on linux/x64Diffs are based on 2,293,437 contexts (933,876 MinOpts, 1,359,561 FullOpts). MISSED contexts: base: 0 (0.00%), diff: 6 (0.00%) Overall (-193,772 bytes)
FullOpts (-193,772 bytes)
Assembly diffs for windows/arm64 ran on linux/x64Diffs are based on 2,376,925 contexts (945,150 MinOpts, 1,431,775 FullOpts). MISSED contexts: base: 5 (0.00%), diff: 11 (0.00%) Overall (-199,184 bytes)
FullOpts (-199,184 bytes)
Assembly diffs for windows/x64 ran on linux/x64Diffs are based on 2,416,970 contexts (937,071 MinOpts, 1,479,899 FullOpts). MISSED contexts: base: 0 (0.00%), diff: 6 (0.00%) Overall (+43,548 bytes)
FullOpts (+43,548 bytes)
Details here Throughput diffsThroughput diffs for linux/arm ran on windows/x86Overall (+0.02% to +0.04%)
FullOpts (+0.03% to +0.04%)
Throughput diffs for windows/x86 ran on windows/x86Overall (+0.04% to +0.05%)
FullOpts (+0.04% to +0.06%)
Details here Throughput diffs for linux/arm64 ran on windows/x64Overall (+0.03% to +0.05%)
MinOpts (-0.00% to +0.01%)
FullOpts (+0.04% to +0.06%)
Throughput diffs for linux/x64 ran on windows/x64Overall (+0.03% to +0.05%)
FullOpts (+0.04% to +0.08%)
Throughput diffs for osx/arm64 ran on windows/x64Overall (+0.03% to +0.05%)
FullOpts (+0.04% to +0.06%)
Throughput diffs for windows/arm64 ran on windows/x64Overall (+0.03% to +0.05%)
FullOpts (+0.04% to +0.06%)
Throughput diffs for windows/x64 ran on windows/x64Overall (+0.04% to +0.05%)
FullOpts (+0.04% to +0.08%)
Details here |
Diff results for #98561Assembly diffsAssembly diffs for linux/arm64 ran on windows/x64Diffs are based on 2,544,344 contexts (1,012,496 MinOpts, 1,531,848 FullOpts). MISSED contexts: base: 0 (0.00%), diff: 6 (0.00%) Overall (-209,660 bytes)
FullOpts (-209,660 bytes)
Assembly diffs for linux/x64 ran on windows/x64Diffs are based on 2,535,365 contexts (984,668 MinOpts, 1,550,697 FullOpts). MISSED contexts: base: 0 (0.00%), diff: 6 (0.00%) Overall (+49,121 bytes)
FullOpts (+49,121 bytes)
Details here Assembly diffs for linux/arm ran on windows/x86Diffs are based on 2,250,511 contexts (832,197 MinOpts, 1,418,314 FullOpts). MISSED contexts: base: 73,582 (3.17%), diff: 73,583 (3.17%) Overall (-109,592 bytes)
FullOpts (-109,592 bytes)
Assembly diffs for windows/x86 ran on windows/x86Diffs are based on 2,354,243 contexts (851,840 MinOpts, 1,502,403 FullOpts). MISSED contexts: base: 0 (0.00%), diff: 9 (0.00%) Overall (+3,263 bytes)
FullOpts (+3,263 bytes)
Details here Throughput diffsThroughput diffs for linux/arm ran on windows/x86Overall (+0.02% to +0.04%)
FullOpts (+0.03% to +0.04%)
Throughput diffs for windows/x86 ran on windows/x86Overall (+0.04% to +0.05%)
FullOpts (+0.04% to +0.06%)
Details here Throughput diffs for linux/arm64 ran on linux/x64Overall (+0.01% to +0.02%)
FullOpts (+0.01% to +0.03%)
Throughput diffs for linux/x64 ran on linux/x64Overall (+0.01% to +0.03%)
FullOpts (+0.01% to +0.05%)
Details here |
…g handles in a relocatable code.
filipnavara
changed the title
filipnavara
changed the title
Diff results for #98561Throughput diffsThroughput diffs for linux/arm64 ran on windows/x64Overall (+0.02% to +0.05%)
FullOpts (+0.03% to +0.05%)
Throughput diffs for linux/x64 ran on windows/x64Overall (+0.02% to +0.05%)
FullOpts (+0.03% to +0.05%)
Throughput diffs for osx/arm64 ran on windows/x64Overall (+0.02% to +0.05%)
MinOpts (-0.01% to +0.00%)
FullOpts (+0.03% to +0.05%)
Throughput diffs for windows/arm64 ran on windows/x64Overall (+0.02% to +0.05%)
MinOpts (-0.01% to +0.00%)
FullOpts (+0.03% to +0.05%)
Throughput diffs for windows/x64 ran on windows/x64Overall (+0.02% to +0.05%)
FullOpts (+0.03% to +0.05%)
Details here |
Diff results for #98561Assembly diffsAssembly diffs for linux/arm64 ran on windows/x64Diffs are based on 2,544,350 contexts (1,012,496 MinOpts, 1,531,854 FullOpts). Overall (-64 bytes)
FullOpts (-64 bytes)
Assembly diffs for linux/x64 ran on windows/x64Diffs are based on 2,535,371 contexts (984,668 MinOpts, 1,550,703 FullOpts). Overall (-44 bytes)
FullOpts (-44 bytes)
Assembly diffs for osx/arm64 ran on windows/x64Diffs are based on 2,293,443 contexts (933,876 MinOpts, 1,359,567 FullOpts). Overall (-64 bytes)
FullOpts (-64 bytes)
Assembly diffs for windows/arm64 ran on windows/x64Diffs are based on 2,376,931 contexts (945,150 MinOpts, 1,431,781 FullOpts). MISSED contexts: 5 (0.00%) Overall (-64 bytes)
FullOpts (-64 bytes)
Assembly diffs for windows/x64 ran on windows/x64Diffs are based on 2,416,976 contexts (937,071 MinOpts, 1,479,905 FullOpts). Overall (-196 bytes)
FullOpts (-196 bytes)
Details here Throughput diffsThroughput diffs for linux/arm ran on windows/x86Overall (+0.01% to +0.04%)
FullOpts (+0.02% to +0.04%)
Throughput diffs for windows/x86 ran on windows/x86Overall (+0.03% to +0.05%)
FullOpts (+0.04% to +0.05%)
Details here |
teo-tsirpanis
added
the
community-contribution
jakobbotsch
approved these changes
Feb 19, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prevent incorrect constant folding of arithmetic operations involving handle in relocatable code.
Fixes #98493