Unify struct arg handling by CarolEidt · Pull Request #18358 · dotnet/coreclr

CarolEidt · 2018-06-07T22:06:06Z

Eliminate unnecessary struct copies, especially on Linux, and reduce code duplication.
Across all targets, use GT_FIELD_LIST to pass promoted structs on stack, and avoid
requiring a copy and/or marking lvDoNotEnregister for those cases.

Unify the specification of multi-reg args:

numRegs now indicates the actual number of reg args (not the size in pointer-size units)
regNums contains all the arg register numbers

Eliminate unnecessary struct copies, especially on Linux, and reduce code duplication. Across all targets, use GT_FIELD_LIST to pass promoted structs on stack, and avoid requiring a copy and/or marking `lvDoNotEnregister` for those cases. Unify the specification of multi-reg args: - numRegs now indicates the actual number of reg args (not the size in pointer-size units) - regNums contains all the arg register numbers

CarolEidt · 2018-06-07T23:50:56Z

This change has zero diffs for x86, x64/windows, arm altjit and arm64 altjit.
For x64/ux, the diffs are:
-7442766 fewer bytes (-3.52% of base), (19596 methods improved, 39 regressed)

A private perf run showed the following improvements on the Devirtualization benchmark (i.e. a factor of 2 for 3 of the 4 sub-benchmarks):
Devirtualization / EqualityComparer, +1.719 (overall)
ValueTupleCompare, +1.023
ValueTupleCompareCached, +2.030
ValueTupleCompareNoOpt, +2.083
ValueTupleCompareWrapped, +2.018

For the remaining benchmarks, there were many numbers in the roughly .9 to 1.1 range that were reported as signficant, but neither crossgen asm-diffs nor superpmi run against both ZapDisable and regular benchmarks on x64/ux showed any significant negative diffs. For the regular run, the overall was:
-36623 fewer bytes (-1.759% of base), 810 methods improved, 7 regressed
The ZapDisable run was similar.
The 7 regressions are:

Microsoft.CodeAnalysis.CSharp.CodeGen.StackOptimizerPass1:VisitArguments (432 to 777 bytes) - A loop was cloned, and the "good" loop is actually larger, due to edge splitting and spilling
Microsoft.CodeAnalysis.PEAssembly:.ctor (342 to 412 bytes) - A loop is cloned so the code is larger, but the loop is 65 bytes, where it was 86 before.
Burgers:GetCalculated3 (1050 to 1052 bytes) - 2 additional spills
System.Linq.EnumerableSorter[KeyValuePair,Int32][System.Collections.Generic.KeyValuePair[System.Int64,System.__Canon],System.Int32]:ComputeKeys (220 to 232 bytes) - An arg is copied differently (not because it's a struct, but because of side-effects, this causes different ordering, different register allocation and slightly larger code)
Microsoft.CodeAnalysis.MetadataReaderExtensions:ReadAssemblyIdentityOrThrow (757 to 768 bytes) - Smaller code before LSRA, but more spill & resolution moves
System.Collections.Generic.ArraySortHelper1[CustomAttributeRow][Microsoft.Cci.MetadataWriter+CustomAttributeRow]:SwapIfGreater(ref,ref,int,int) ` (177 to 185 bytes) - Because arg1 doesn't get copied, it has a longer lifetime, and winds up getting spilled
Microsoft.CodeAnalysis.CSharp.Symbols.AnonymousTypeManager:AssignTemplatesNamesAndCompile (1533 to 1536 bytes) - Similar to above - one less var, different live ranges, 1 more resolution move and 1 more split edge

CarolEidt · 2018-06-07T23:52:27Z

@dotnet/jit-contrib PTAL
This eliminate some of the x64/ux-specific code, but makes only marginal progress toward the design described in https://github.com/dotnet/coreclr/blob/master/Documentation/design-docs/struct-abi.md.

mikedn · 2018-06-08T15:29:55Z

-    baseVarNum = compiler->lvaFirstStackIncomingArgNum;
-
-    if (compiler->lvaFirstStackIncomingArgNum != BAD_VAR_NUM)
+    // Iterate over all the local variables in the Lcl var table.


Is it me or this comment is wrong? The for loop below seems to iterate only over the args.

You're correct - the comment is wrong. In my defense, it isn't new (though the diffs seem to think so), it was just moved from below.
I'll correct it.

sdmaclea

LGTM (Looked only at the general idea and impact to arm64)

jashook

Lgtm have some nits

jashook · 2018-06-08T16:15:21Z

+    {
+        // If this is passed as a floating type, use that.
+        // Otherwise, we'll use the general case - we don't want to use the "EightByteType"
+        // directly, because it won't preserve small types.


This comment seems to imply that the classification returned by Compiler::EightByteType is incorrect, or at least being deprecated. If this is the case, I think it is worth adding a comment to Compiler::EightByteType explaining its limitations.

I think I should just reword the comment. It's not really a limitation of the "EightByteType", it's that it returns TYP_INT for any integer type <= 4 bytes. I could change that method, but other callers depend on that behavior.

jashook · 2018-06-08T16:18:05Z

-        // Set 'useType' to the type of the first eightbyte item
+        // We can't pass this as a primitive type.
+    }
+    else if (structDesc.passedInRegisters && varTypeIsFloating(GetEightByteType(structDesc, 0)))


Seems easier to directly use structDesc.eightByteClassifications[0] == SystemVClassificationTypeSSE even if it is ugly it saves a few calls, one that is redundant.

OK, I'm not totally comfortable with that, because it assumes that the structDesc is initialized (I realize it is, but it's not obvious from this context).

jashook · 2018-06-08T16:24:32Z


                // The case of (structDesc.eightByteCount == 1) should have already been handled
-                if (structDesc.eightByteCount > 1)
+                if ((structDesc.eightByteCount > 1) || !structDesc.passedInRegisters)


What case does adding || !structDesc.passedInRegisters? It seems like the comment is correct, by this point any Arg that is <= 8bytes should have been handled and any arg that is > 8 bytes but <= 16 bytes will be correctly addressed with structDesc.eightByteCount > 1 regardless of whether structDesc.passedInRegisters.

No, I don't think so. I believe that if passedInRegisters is false, the eightByteCount will always be 0 - in which case we still need to set this as being passed by value. It's only the pass by reference case that should not fall into this then clause.

Makes sense thank you for clarifying.

jashook · 2018-06-08T16:26:53Z

+private:
+    regNumberSmall regNums[MAX_ARG_REG_COUNT]; // The registers to use when passing this argument, set to REG_STK for
+                                               // arguments passed on
+                                               // the stack


Nit: condense to one line:

// arguments passed on the stack

jashook · 2018-06-08T16:49:58Z

            // In such case the fgArgTabEntry keeps track of whether the original node (before morphing)
            // was a struct and the struct classification.
-            isStructArg = fgEntryPtr->isStruct;
+            isStructArg = argEntry->isStruct;


Nit: It may be preferable to declare isStructArg twice. To avoid any potential problems with bool isStructArg being uninitialized if someone changes this in the future.

bool isStructArg = argEntry->isStruct;
and in !remorphing
bool isStructArg = varTypeIsStruct(argx);

That won't work, because we need the value below this if-then-else.

I see, that is unfortunate, ok

jashook · 2018-06-08T16:57:17Z

                        !isPow2(originalSize))                    // size is 3,5,6 or 7 bytes
                    {
-                        if (argObj->gtObj.gtOp1->IsVarAddr()) // Is the source a LclVar?
+                        // if (argObj->gtObj.gtOp1->IsVarAddr()) // Is the source a LclVar?


Why is this commented out? Is it necessary? If not can it just be deleted? As is it is a little hard to follow.

Oops - I had convinced myself that this didn't need to be limited to that case, so I commented it out to test my hypothesis. Then I failed to remove it.

jashook · 2018-06-08T16:58:07Z

                        {
-                            // For ARM64 we pass structs that are 3,5,6,7 bytes in size
-                            // we can read 4 or 8 bytes from the LclVar to pass this arg
+                            // For ARM64 we pass structs that are 3,5,6,7 bytes in size in registers.


Nit: update comment, this now is for arm64 or unix amd64.

Deleted the comment, as it is redundant

jashook · 2018-06-08T17:03:51Z

                            size = roundupSize / TARGET_POINTER_SIZE; // Normalize size to number of pointer sized items
                        }
                    }
+#endif


Nit: Add comment to endif

#endif // !UNIX_AMD64_ABI

jashook · 2018-06-08T17:05:01Z

+#ifdef FEATURE_HFA
+                if (!passUsingFloatRegs)
+                {
+                    // Note on Arm32 a HFA is passed in int regs for varargs


This currently is currently correct for Arm32 and Arm64

Also note that this is Windows specific

briansull · 2018-06-08T17:50:50Z

-            // We are iterating over the arguments only.
-            assert(varDsc->lvIsParam);
+        // We are iterating over the arguments only.
+        assert(varDsc->lvIsParam);


With /GSChecks we can make a "shadow" copy of an incoming struct argument.
Basically we are copying it from the incoming Arg Space into a new struct local that is placed so that
it will catch any buffer overruns into its storage. Something to think about with this change.

Thanks, Brian. Just to make sure I understand - this would be a "new" local though, right? So that the original varDsc would still have the original offset. If that's the case, then I don't think that would impact this. Otherwise, this would have a problem because it assumes that they are laid out in order.

Yes it is a new local, I'm not sure about all the details here, but the IL references to the arg are rewrtten to refer to the new local.

There was a recent bug where we struct promotion interacted badly and it and end up with reference to both the original copy and to the individual promoted fields.

A fix and test case for the issue is here: #17329

briansull · 2018-06-08T17:51:09Z

+    }
+}
+#endif // !_TARGET_X86_
+


Nice refactoring here.

CarolEidt · 2018-06-08T20:09:14Z

The arm failures in the previous round were due to an overly aggressive assert in checkIsStruct() for ARM where a stack-passed struct is passed as TYP_DOUBLE.
I believe I've addressed all the feedback.
Once the CI tests pass, I'll run pri1 tests.

jashook · 2018-06-11T22:30:46Z

@dotnet-bot test Ubuntu arm Cross Checked Innerloop Build and Test
@dotnet-bot test Windows_NT arm64 Cross Checked normal Build and Test
@dotnet-bot test Windows_NT arm Cross Checked normal Build and Test
@dotnet-bot test Windows_NT x64 Build and Test
@dotnet-bot test Windows_NT x86 Checked Build and Test
@dotnet-bot test Ubuntu x64 Build and Test

jashook · 2018-06-11T22:35:32Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

…ckfilled args.

CarolEidt · 2018-06-12T05:23:13Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

jashook · 2018-06-12T05:30:04Z

@dotnet-bot test Windows_NT arm64 Cross Checked normal Build and Test
@dotnet-bot test Windows_NT arm Cross Checked normal Build and Test
@dotnet-bot test Windows_NT x64 Build and Test
@dotnet-bot test Windows_NT x86 Checked Build and Test
@dotnet-bot test Ubuntu x64 Build and Test

CarolEidt · 2018-06-12T14:11:55Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

* Unify struct arg handling Eliminate unnecessary struct copies, especially on Linux, and reduce code duplication. Across all targets, use GT_FIELD_LIST to pass promoted structs on stack, and avoid requiring a copy and/or marking `lvDoNotEnregister` for those cases. Unify the specification of multi-reg args: - numRegs now indicates the actual number of reg args (not the size in pointer-size units) - regNums contains all the arg register numbers Commit migrated from dotnet/coreclr@d28957d

CarolEidt force-pushed the fgMorphArgs1 branch from 2bca0f1 to d64f186 Compare June 7, 2018 23:01

jashook self-requested a review June 8, 2018 05:05

mikedn reviewed Jun 8, 2018

View reviewed changes

sdmaclea approved these changes Jun 8, 2018

View reviewed changes

jashook approved these changes Jun 8, 2018

View reviewed changes

briansull reviewed Jun 8, 2018

View reviewed changes

PR Review Feedback

657bf24

Fix overly agressive assert

95deaa0

Move the check for split args after the check for non-standard and ba…

f80180a

…ckfilled args.

CarolEidt merged commit d28957d into dotnet:master Jun 12, 2018

jashook mentioned this pull request Jun 12, 2018

[Windows|Arm64|VarArgs] Correctly pass HFA arguments #18364

Merged

jkotas mentioned this pull request Jun 14, 2018

Update CoreClr, CoreFx, CoreSetup, ProjectNTfs, ProjectNTfsTestILC, Standard to preview1-26613-04, preview1-26613-07, preview1-26613-01, beta-26613-00, beta-26613-00, servicing-26419-02, respectively (master) dotnet/corefx#30338

Closed

CarolEidt mentioned this pull request Aug 2, 2018

Handle COMMA nodes when morphing struct args #19234

Merged

CarolEidt deleted the fgMorphArgs1 branch August 31, 2018 17:49

AndyAyersMS mentioned this pull request Jan 12, 2019

Add Utf8JsonWriter along with unit tests dotnet/corefx#34425

Merged

CarolEidt mentioned this pull request Feb 22, 2019

Fix condition for calling EvalArgsToTemps #22791

Merged

adityamandaleeka mentioned this pull request Jan 31, 2020

Incorrect up-sizing for non-power-of-2 structs that are not LclVars dotnet/runtime#10833

Closed

Conversation

CarolEidt commented Jun 7, 2018

Uh oh!

CarolEidt commented Jun 7, 2018

Uh oh!

CarolEidt commented Jun 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdmaclea left a comment

Choose a reason for hiding this comment

Uh oh!

jashook left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briansull Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarolEidt commented Jun 8, 2018

Uh oh!

jashook commented Jun 11, 2018

Uh oh!

jashook commented Jun 11, 2018

Uh oh!

CarolEidt commented Jun 12, 2018

Uh oh!

jashook commented Jun 12, 2018

Uh oh!

CarolEidt commented Jun 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

briansull Jun 8, 2018 •

edited

Loading