-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Pass HFA/HVA in registers #62623
Pass HFA/HVA in registers #62623
Conversation
…when they are not passed on SIMD registers" in src/coreclr/jit/morph.cpp
Tagging subscribers to this area: @JulieLeeMSFT Issue Detailsnull
|
/azp run runtime-coreclr jitstress |
Azure Pipelines successfully started running 1 pipeline(s). |
@kunalspathak PTAL |
@@ -4284,11 +4284,7 @@ GenTree* Compiler::fgMorphMultiregStructArg(GenTree* arg, fgArgTabEntry* fgEntry | |||
var_types type[MAX_ARG_REG_COUNT] = {}; // TYP_UNDEF = 0 | |||
|
|||
hfaType = fgEntryPtr->GetHfaType(); | |||
if (varTypeIsValidHfaType(hfaType) |
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.
I changed the condition:
- I believe if was incorrectly mentioning
!HOST_UNIX
instead of specifying that the target is Windows. - My understanding is we wanted to check here is that the argument is passed in SIMD register (win-arm64 varargs prohibits using of SIMD registers).
- However, I believe that checking for
!fgEntryPtr->IsVararg()
was not enough - we should've checked for a stronger condition that the function has any vararg arguments at all - https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions?view=msvc-160#addendum-variadic-functions ?
An example of the diff in the libraries code: diff --git a/base/178576.dasm b/diff/178576.dasm
index 7a4d5361b1c..12dc7570806 100644
--- a/base/178576.dasm
+++ b/diff/178576.dasm
@@ -6,45 +6,37 @@
; No matching PGO data
; Final local variable assignments
;
-; V00 arg0 [V00 ] ( 6, 6 ) struct ( 8) [fp+28H] HFA(float) do-not-enreg[SFA] multireg-arg single-def
-; V01 arg1 [V01 ] ( 6, 6 ) struct ( 8) [fp+20H] HFA(float) do-not-enreg[SFA] multireg-arg single-def
+;* V00 arg0 [V00 ] ( 0, 0 ) struct ( 8) zero-ref HFA(float) multireg-arg single-def
+;* V01 arg1 [V01 ] ( 0, 0 ) struct ( 8) zero-ref HFA(float) multireg-arg single-def
;# V02 OutArgs [V02 ] ( 1, 1 ) lclBlk ( 0) [sp+00H] "OutgoingArgSpace"
;* V03 tmp1 [V03 ] ( 0, 0 ) struct ( 8) zero-ref HFA(float) multireg-ret "Return value temp for multireg return"
-; V04 tmp2 [V04,T01] ( 5, 5 ) float -> [fp+28H] do-not-enreg[] single-def V00.x(offs=0x00) P-DEP "field V00.x (fldOffset=0x0)"
-; V05 tmp3 [V05,T02] ( 5, 5 ) float -> [fp+2CH] do-not-enreg[] single-def V00.y(offs=0x04) P-DEP "field V00.y (fldOffset=0x4)"
-; V06 tmp4 [V06,T03] ( 5, 5 ) float -> [fp+20H] do-not-enreg[] single-def V01.width(offs=0x00) P-DEP "field V01.width (fldOffset=0x0)"
-; V07 tmp5 [V07,T04] ( 5, 5 ) float -> [fp+24H] do-not-enreg[] single-def V01.height(offs=0x04) P-DEP "field V01.height (fldOffset=0x4)"
+; V04 tmp2 [V04,T01] ( 2, 2 ) float -> d0 single-def V00.x(offs=0x00) P-INDEP "field V00.x (fldOffset=0x0)"
+; V05 tmp3 [V05,T02] ( 2, 2 ) float -> d1 single-def V00.y(offs=0x04) P-INDEP "field V00.y (fldOffset=0x4)"
+; V06 tmp4 [V06,T03] ( 2, 2 ) float -> d2 single-def V01.width(offs=0x00) P-INDEP "field V01.width (fldOffset=0x0)"
+; V07 tmp5 [V07,T04] ( 2, 2 ) float -> d3 single-def V01.height(offs=0x04) P-INDEP "field V01.height (fldOffset=0x4)"
; V08 tmp6 [V08,T05] ( 2, 2 ) float -> d0 V03.x(offs=0x00) P-INDEP "field V03.x (fldOffset=0x0)"
; V09 tmp7 [V09,T06] ( 2, 2 ) float -> d1 V03.y(offs=0x04) P-INDEP "field V03.y (fldOffset=0x4)"
; V10 tmp8 [V10,T00] ( 3, 3 ) struct ( 8) [fp+18H] HFA(float) do-not-enreg[SFR] multireg-ret "Return value temp for multi-reg return (rejected tail call)."
;
-; Lcl frame size = 32
+; Lcl frame size = 16
G_M36202_IG01: ; gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, nogc <-- Prolog IG
- stp fp, lr, [sp,#-48]!
+ stp fp, lr, [sp,#-32]!
mov fp, sp
- str s0, [fp,#40]
- str s1, [fp,#44]
- str s2, [fp,#32]
- str s3, [fp,#36]
- ;; bbWeight=1 PerfScore 5.50
+ ;; bbWeight=1 PerfScore 1.50
G_M36202_IG02: ; gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
- ldr s0, [fp,#40]
- ldr s1, [fp,#44]
- ldr s2, [fp,#32]
- ldr s3, [fp,#36]
bl System.Drawing.PointF:Add(System.Drawing.PointF,System.Drawing.SizeF):System.Drawing.PointF
str s0, [fp,#24]
str s1, [fp,#28]
ldr s0, [fp,#24]
ldr s1, [fp,#28]
- ;; bbWeight=1 PerfScore 15.00
+ ;; bbWeight=1 PerfScore 7.00
G_M36202_IG03: ; , epilog, nogc, extend
- ldp fp, lr, [sp],#48
+ ldp fp, lr, [sp],#32
ret lr
;; bbWeight=1 PerfScore 2.00
-; Total bytes of code 68, prolog size 8, PerfScore 29.30, instruction count 17, allocated bytes for code 68 (MethodHash=96e07295) for method System.Drawing.PointF:op_Addition(System.Drawing.PointF,System.Drawing.SizeF):System.Drawing.PointF
+; Total bytes of code 36, prolog size 8, PerfScore 14.10, instruction count 9, allocated bytes for code 36 (MethodHash=96e07295) for method System.Drawing.PointF:op_Addition(System.Drawing.PointF,System.Drawing.SizeF):System.Drawing.PointF
; ============================================================ |
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.
I was confused by the title, because "Pass HFA/HVA in registers" implies to me that you are changing the calling convention / ABI, but actually, you're just changing how HFA/HVA arguments are handled in setting up for calls, not where the values live in argument registers.
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. Nice wins!
Sorry for the confusion. I should've named the change as "Allow promoted fields of HFA/HVAs to stay in registers when possible". |
Consider the following code snippet
PassHaStructsInRegs.cs
Note that all HFA/HVA values are stored to memory and, then, reloaded back to the same registers when the values are passed as argument to a function as it can be seen in the following output:
Currently
The change adds support in
fgMorphMultiregStructArg()
to allow in a case when such HFA/HVA value is promoted to be represented asGT_FIELD_LIST
and, effectively, be passed in registers.After #62623
Note, however, that the HFA/HVA values with 4 fields are still passed through the memory - currently we don't promote struct with more than 3 fields if none of the fields is accessed. I plan to work on this in a follow-up PR.