-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[arm64] JIT: Make 0.0 containable for fcmp #61617
Conversation
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsA quick fix for: void Test(double x)
{
if (x < 0)
Console.WriteLine();
} codegen diff: ; Method MandelBrot:Test(double):this
G_M9914_IG01: ;; offset=0000H
A9BF7BFD stp fp, lr, [sp,#-16]!
910003FD mov fp, sp
;; bbWeight=1 PerfScore 1.50
G_M9914_IG02: ;; offset=0008H
- 4F00E410 movi v16.16b, #0x00
- 1E702000 fcmp d0, d16
+ 1E602008 fcmp d0, #0.0
54000042 bhs G_M9914_IG04
;; bbWeight=1 PerfScore 3.00
G_M9914_IG03: ;; offset=0010H
94000000 bl System.Console:WriteLine()
;; bbWeight=0.50 PerfScore 0.50
G_M9914_IG04: ;; offset=0014H
A8C17BFD ldp fp, lr, [sp],#16
D65F03C0 ret lr
;; bbWeight=1 PerfScore 2.00
; Total bytes of code: 28 For arm32 it requires more effort so I decided to leave that TODO (it was there before my changes)
|
benchmarks.run.windows.arm64.checked.mch:
Detail diffs
coreclr_tests.pmi.windows.arm64.checked.mch:
Detail diffs
libraries.crossgen2.windows.arm64.checked.mch:
Detail diffs
libraries.pmi.windows.arm64.checked.mch:
Detail diffs
libraries_tests.pmi.windows.arm64.checked.mch:
Detail diffs
|
@@ -3585,7 +3584,7 @@ void CodeGen::genCodeForCompare(GenTreeOp* tree) | |||
assert(!op1->isContained()); | |||
assert(op1Type == op2Type); | |||
|
|||
if (op2->IsIntegralConst(0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EgorBo So, effectively, this was always dead code, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@echesakovMSFT yep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Can you share what the regressions look like?
some weird artifacts, I think I see them every time I make some changes which produce diffs |
I think it will be good to understand where are they coming from. Looks like in both cases, there is a "Inline arg" that was previously had zero reference but now we start using it? |
@EgorBo Is it possible this could have broken Mono JitIntrinsics tests? https://dev.azure.com/dnceng/public/_build/results?buildId=1472243&view=ms.vss-test-web.build-test-results-tab&runId=42217752&resultId=102746&paneView=dotnet-dnceng.dnceng-build-release-tasks.helix-test-information-tab |
@agocke definitely not possible, this PR only touches two jit .cpp files which are not used in mono |
A quick fix for:
codegen diff:
For arm32 it requires more efforts so I decided to leave that TODO (it was there before my changes)
@dotnet/jit-contrib @echesakovMSFT