Uh oh!
There was an error while loading. Please reload this page.
JIT: some enhancements to redundant branch opts - #60732
Merged
Merged
Conversation
Handle cases where the dominating compare is the reverse of the compare we're trying to optimize. For example, if `(x > y)` dominates `(y <= x)` we may be able to optimize the dominated compare. Addresses aspects of dotnet#48115.
ghost
commented
Oct 21, 2021
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsHandle cases where the dominating compare is the reverse of the compare Addresses aspects of #48115.
|
AndyAyersMS
commented
Oct 21, 2021
MemberAuthor
cc @dotnet/jit-contrib Small number of regressions, mostly RA/CSE differences. aspnet.run.windows.x64.checked.mch:Detail diffsbenchmarks.run.windows.x64.checked.mch:Detail diffscoreclr_tests.pmi.windows.x64.checked.mch:Detail diffslibraries.crossgen2.windows.x64.checked.mch:Detail diffslibraries.pmi.windows.x64.checked.mch:Detail diffslibraries_tests.pmi.windows.x64.checked.mch:Detail diffsTypical diff ; Assembly listing for method System.Data.SqlClient.TdsParser:set_CurrentTransaction(System.Data.SqlClient.SqlInternalTransaction):this
; ...
; V00 this [V00,T01] ( 4, 3.50) ref -> rcx this class-hnd single-def
; V01 arg1 [V01,T00] ( 5, 3.50) ref -> rdx class-hnd single-def
;# V02 OutArgs [V02 ] ( 1, 1 ) lclBlk ( 0) [rsp+00H] "OutgoingArgSpace"
-; V03 cse0 [V03,T02] ( 3, 2.50) ref -> rax "CSE - aggressive"
;
; Lcl frame size = 0
@@ -17,23 +16,19 @@ G_M27428_IG01: ; gcrefRegs=00000000 {}, byrefRegs=00000000 {}, byref, nog
;; bbWeight=1 PerfScore 0.00
G_M27428_IG02: ; gcrefRegs=00000006 {rcx rdx}, byrefRegs=00000000 {}, byref, isz
; gcrRegs +[rcx rdx]
- mov rax, gword ptr [rcx+48]- ; gcrRegs +[rax]- test rax, rax+ cmp gword ptr [rcx+48], 0
jne SHORT G_M27428_IG04
- ;; bbWeight=1 PerfScore 3.25-G_M27428_IG03: ; gcrefRegs=00000007 {rax rcx rdx}, byrefRegs=00000000 {}, byref, isz+ ;; bbWeight=1 PerfScore 4.00+G_M27428_IG03: ; gcrefRegs=00000006 {rcx rdx}, byrefRegs=00000000 {}, byref, isz
test rdx, rdx
jne SHORT G_M27428_IG05
- ;; bbWeight=0.50 PerfScore 0.62-G_M27428_IG04: ; gcrefRegs=00000007 {rax rcx rdx}, byrefRegs=00000000 {}, byref, isz- test rax, rax- je SHORT G_M27428_IG06+ jmp SHORT G_M27428_IG06+ ;; bbWeight=0.50 PerfScore 1.62+G_M27428_IG04: ; gcrefRegs=00000006 {rcx rdx}, byrefRegs=00000000 {}, byref, isz
test rdx, rdx
jne SHORT G_M27428_IG06
- ;; bbWeight=0.50 PerfScore 1.25+ ;; bbWeight=0.50 PerfScore 0.62
G_M27428_IG05: ; gcrefRegs=00000006 {rcx rdx}, byrefRegs=00000000 {}, byref
- ; gcrRegs -[rax]
lea rcx, bword ptr [rcx+48]
; gcrRegs -[rcx]
; byrRegs +[rcx]
@@ -48,7 +43,7 @@ G_M27428_IG07: ; , epilog, nogc, extend
ret
;; bbWeight=1 PerfScore 1.00
-; Total bytes of code 35, prolog size 0, PerfScore 10.63, instruction count 13, allocated bytes for code 35 (MethodHash=d2cb94db) for method System.Data.SqlClient.TdsParser:set_CurrentTransaction(System.Data.SqlClient.SqlInternalTransaction):this+; Total bytes of code 30, prolog size 0, PerfScore 11.25, instruction count 11, allocated bytes for code 30 (MethodHash=d2cb94db) for method System.Data.SqlClient.TdsParser:set_CurrentTransaction(System.Data.SqlClient.SqlInternalTransaction):thisOne interesting regression, in |
Member
Nice! What about this case: both conditions are absolutely the same, just the order of operands is different. also, I assume this opt won't work for |
AndyAyersMS
commented
Oct 21, 2021
MemberAuthor
Not if sure all these cases are mapped to one standard form -- so probably worth looking at in more depth.
Good point, we should exclude fp compares here. Let's see if we get any failures from this in existing testing. |
SingleAccretion
commented
Oct 21, 2021
Contributor
A note to readers: this is because we do not model floating-pont |
AndyAyersMS
commented
Oct 21, 2021
MemberAuthor
GCC build seems to be previously broken: See #60225 (comment) |
MemberAuthor
@EgorBo I am also seeing the case you noted above (see below, messed-up example edited out) |
MemberAuthor
[edit: removed obsolete / incorrect info] |
SingleAccretion
commented
Oct 21, 2021
Contributor
Unfortunately it seems this would require "further work", as it trips up the fragile range check elimination. |
AndyAyersMS
commented
Oct 22, 2021
MemberAuthor
Above summary of adding support for swapped compares wasn't quite implemented properly. Updated diff of diffs is -131 total methods with Code Size differences (128 improved, 3 regressed), 14 unchanged.-193 total methods with Code Size differences (171 improved, 22 regressed), 3 unchanged.-421 total methods with Code Size differences (418 improved, 3 regressed), 5 unchanged.-363 total methods with Code Size differences (343 improved, 20 regressed), 8 unchanged.-1197 total methods with Code Size differences (1112 improved, 85 regressed), 45 unchanged.-1241 total methods with Code Size differences (1156 improved, 85 regressed), 35 unchanged.+134 total methods with Code Size differences (131 improved, 3 regressed), 14 unchanged.+210 total methods with Code Size differences (185 improved, 25 regressed), 5 unchanged.+385 total methods with Code Size differences (381 improved, 4 regressed), 5 unchanged.+538 total methods with Code Size differences (519 improved, 19 regressed), 8 unchanged.+1455 total methods with Code Size differences (1368 improved, 87 regressed), 53 unchanged.+1281 total methods with Code Size differences (1196 improved, 85 regressed), 39 unchanged.One interesting example: We now produce the following odd looking bit of code... G_M030_IG01: ; gcVars=0000000000000000 {}, gcrefRegs=00000000 {}, byrefRegs=00000000 {}, gcvars, byref, nogc <-- Prolog IGmovrax,rcx ; gcrRegs +[rax] ;; bbWeight=1 PerfScore 0.25G_M030_IG02: ; gcrefRegs=00000001 {rax}, byrefRegs=00000000 {}, byref, iszcmp dword ptr [rax+24],0jg SHORT G_M030_IG04 ;; bbWeight=1 PerfScore 4.00G_M030_IG03: ; gcrefRegs=00000001 {rax}, byrefRegs=00000000 {}, byref, epilog, nogcret ;; bbWeight=0.50 PerfScore 0.50G_M030_IG04: ; gcrefRegs=00000001 {rax}, byrefRegs=00000000 {}, byref, epilog, nogcret |
AndyAyersMS
commented
Oct 22, 2021
MemberAuthor
Updated diff summary aspnet.run.windows.x64.checked.mch:Detail diffsbenchmarks.run.windows.x64.checked.mch:Detail diffscoreclr_tests.pmi.windows.x64.checked.mch:Detail diffslibraries.crossgen2.windows.x64.checked.mch:Detail diffslibraries.pmi.windows.x64.checked.mch:Detail diffslibraries_tests.pmi.windows.x64.checked.mch:Detail diffs |
AndyAyersMS
commented
Oct 22, 2021
MemberAuthor
@dotnet/jit-contrib ping |
BruceForstall
commented
Oct 22, 2021
Contributor
I must be missing something. In your example above, if I pass |
BruceForstall
left a comment
Contributor
There was a problem hiding this comment.
The code looks good, but the discussion of the F example seems wrong.
| // | ||
| // Arguments: | ||
| // vn - vn to base things on | ||
| // vnk - whether the new vn should swap, reverse, or both |
Contributor
There was a problem hiding this comment.
Suggested change
| //vnk - whether the new vn should swap, reverse, or both | |
| //vrk - whether the new vn should swap, reverse, or both |
| // vn for reversed/swapped comparsion, or NoVN. | ||
| // | ||
| // Note: | ||
| // If "vn" corresponds to (x > y), the resulting VN correponds to |
Contributor
There was a problem hiding this comment.
Suggested change
| // If "vn" corresponds to (x > y), the resulting VN correponds to | |
| // If "vn" corresponds to (x > y), the resulting VN corresponds to: |
Comment on lines
152
to
155
| // Note we could also infer the tree relop's value from similar relops higher in the dom tree. | ||
| // For example, (x >= 0) dominating (x > 0), or (x < 0) dominating (x > 0). | ||
| // | ||
| // That is left as a future enhancement. |
Contributor
There was a problem hiding this comment.
Is this comment obsolete now? (Or in need of updating to match the new code)
MemberAuthor
There was a problem hiding this comment.
I'll update it -- there are still cases we don't get, like the first one listed.
MemberAuthor
Sorry about that. My example above was indeed messed up.... here's a corrected one that matches Egor's snippet staticintF3(intx,inty){intr=0;if(x>y){r+=1;if(y>=x)r+=2;}returnr;};;; base; Assembly listing for method X:F3(int,int):int; Emitting BLENDED_CODE for X64 CPU with AVX - Windows; optimized code; rsp based frame; partially interruptible; No PGO data; Final local variable assignments;; V00 arg0 [V00,T00] ( 4, 3.50) int -> rcx single-def; V01 arg1 [V01,T01] ( 4, 3.50) int -> rdx single-def; V02 loc0 [V02,T02] ( 4, 3 ) int -> rax ;# V03 OutArgs [V03 ] ( 1, 1 ) lclBlk ( 0) [rsp+00H] "OutgoingArgSpace";; Lcl frame size = 0G_M37129_IG01: ;; offset=0000H ;; bbWeight=1 PerfScore 0.00G_M37129_IG02: ;; offset=0000H 33C0 xoreax,eax 3BCA cmpecx,edx 7E0E jle SHORT G_M37129_IG04 ;; bbWeight=1 PerfScore 1.50G_M37129_IG03: ;; offset=0006H B801000000 moveax,1 3BD1 cmpedx,ecx 7C05 jl SHORT G_M37129_IG04 B803000000 moveax,3 ;; bbWeight=0.50 PerfScore 0.88G_M37129_IG04: ;; offset=0014H C3 ret ;; bbWeight=1 PerfScore 1.00; Total bytes of code 21, prolog size 0, PerfScore 5.48, instruction count 8, allocated bytes for code 21 (MethodHash=ddcb6ef6) for method X:F3(int,int):int;;; diff; Assembly listing for method X:F3(int,int):int; Emitting BLENDED_CODE for X64 CPU with AVX - Windows; optimized code; rsp based frame; partially interruptible; No PGO data; Final local variable assignments;; V00 arg0 [V00,T00] ( 3, 3 ) int -> rcx single-def; V01 arg1 [V01,T01] ( 3, 3 ) int -> rdx single-def; V02 loc0 [V02,T02] ( 3, 2.50) int -> rax ;# V03 OutArgs [V03 ] ( 1, 1 ) lclBlk ( 0) [rsp+00H] "OutgoingArgSpace";; Lcl frame size = 0G_M37129_IG01: ;; offset=0000H ;; bbWeight=1 PerfScore 0.00G_M37129_IG02: ;; offset=0000H 33C0 xoreax,eax 3BCA cmpecx,edx 7E05 jle SHORT G_M37129_IG04 ;; bbWeight=1 PerfScore 1.50G_M37129_IG03: ;; offset=0006H B801000000 moveax,1 ;; bbWeight=0.50 PerfScore 0.12G_M37129_IG04: ;; offset=000BH C3 ret ;; bbWeight=1 PerfScore 1.00; Total bytes of code 12, prolog size 0, PerfScore 3.83, instruction count 5, allocated bytes for code 12 (MethodHash=ddcb6ef6) for method X:F3(int,int):int |
BruceForstall
approved these changes
Oct 23, 2021
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
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.
Handle cases where the dominating compare is the reverse of the compare
we're trying to optimize. For example, if
(x > y)dominates(y <= x)we may be able to optimize the dominated compare.
Addresses aspects of #48115.