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
Use GT_NULLCHECK for unconsumed indirections. #32641
Use GT_NULLCHECK for unconsumed indirections. #32641
Conversation
All unconsumed indirections are now GT_NULLCHECKs, which allows optFoldNullChecks to remove more redundant null checks.
Framework diffs (x64 pmi):
|
Benchmarks diffs (x64 pmi):
|
Typical diff (from System.Data.SqlClient.dll): ; Assembly listing for method MemoryRecordBuffer:GetBoolean(SmiEventSink,int):bool:this
; Emitting BLENDED_CODE for X64 CPU with AVX - Windows
; optimized code
; rsp based frame
; partially interruptible
;
; Lcl frame size = 40
G_M45197_IG01:
sub rsp, 40
;; bbWeight=1 PerfScore 0.25
G_M45197_IG02:
mov rax, gword ptr [rcx+8]
cmp r8d, dword ptr [rax+8]
jae SHORT G_M45197_IG04
movsxd rdx, r8d
mov rax, gword ptr [rax+8*rdx+16]
- mov edx, dword ptr [rax]
movzx rax, byte ptr [rax+32]
+ ;; bbWeight=1 PerfScore 9.25
- ;; bbWeight=1 PerfScore 11.25
G_M45197_IG03:
add rsp, 40
ret
;; bbWeight=1 PerfScore 1.25
G_M45197_IG04:
call CORINFO_HELP_RNGCHKFAIL
int3
;; bbWeight=0 PerfScore 0.00
-; Total bytes of code 39, prolog size 4, PerfScore 16.65, (MethodHash=34c94f73) for method MemoryRecordBuffer:GetBoolean(SmiEventSink,int):bool:this
+; Total bytes of code 37, prolog size 4, PerfScore 14.45, (MethodHash=34c94f73) for method MemoryRecordBuffer:GetBoolean(SmiEventSink,int):bool:this
|
@dotnet/jit-contrib @AndyAyersMS PTAL |
// COMMA(COMMA(tmp = "this", deref(tmp)), tmp) | ||
thisPtr = gtNewOperNode(GT_COMMA, vt, asg, gtNewLclvNode(lclNum, vt)); | ||
} | ||
else | ||
{ | ||
// thisPtr = COMMA(deref("this"), "this") | ||
GenTree* ind = gtNewOperNode(GT_IND, TYP_INT, thisPtr); |
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.
Was it important that sometimes we were dereferencing TYP_INT
and sometimes TYP_I_IMPL
?
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.
Similar question as @sandreenko -- what factors come into play when choosing BYTE for the type used in GT_NULLCHECK
?
The non-struct case in box removal might warrant similar treatment, but I suspect boxing from scalar indirs is pretty rare.
Otherwise, looks good.
This change will fix half cases from #32248 (when |
The type we set on runtime/src/coreclr/src/jit/codegenxarch.cpp Lines 4049 to 4056 in 87bb243
runtime/src/coreclr/src/jit/codegenarmarch.cpp Lines 1546 to 1559 in 8079a27
|
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
I tried that when |
@AndyAyersMS I answered your comments. |
All unconsumed indirections are now GT_NULLCHECKs, which
allows optFoldNullChecks to remove more redundant null checks.