Fix for bugs related to clasifying System.TypedReference - issue #2854. #2860

LLITCHEV · 2016-01-26T06:52:28Z

It was a long standing assumption in the Jit that a field of a struct can
never be of ByRef type. It turns out this assumption is not entirely
holding.
The System.TypedReference is a very special type that is a struct, defined
to have two IntPtr fields. When the VM instantiates this type it changes
the type of the first field to ByRef.
This change closes a GC hole in the passing this struct by value on the
stack. It adds support to the classification algorithm for ByRef
references. It also uses the tree types for generating the code that
places struct field on the stack by value for passin to a callee.

LLITCHEV · 2016-01-26T14:38:24Z

test OSX x64 Release Build and Test please

LLITCHEV · 2016-01-26T18:43:28Z

@jkotas @janvorli @sivarv PTAL
/cc @dotnet/jit-contrib

CarolEidt · 2016-01-26T18:56:31Z

src/inc/corinfo.h

@@ -276,12 +276,14 @@ enum SystemVClassificationType : unsigned __int8
    SystemVClassificationTypeMemory             = 3,
    SystemVClassificationTypeInteger            = 4,
    SystemVClassificationTypeIntegerReference   = 5,
-    SystemVClassificationTypeSSE                = 6,
+    SystemVClassificationTypeIntegerByReference = 6,


Minor point - but I think these names confusing. It sounds like it's describing a integer being passed by reference. Also, "byref" is a specific term used in the CLI and so it would be more accurate to use that term. I think it should really be ByRefAsInteger (and the previous one "ReferenceAsInteger"), but at least change "ByReference" to "ByRef".

LLITCHEV · 2016-01-28T17:48:09Z

Comments addressed. PTAL.

CarolEidt · 2016-01-28T19:01:15Z

src/jit/codegencommon.cpp

-                     (structDesc.eightByteClassifications[slotCounter] == SystemVClassificationTypeIntegerReference))) ||
-                     (doingFloat && structDesc.eightByteClassifications[slotCounter] == SystemVClassificationTypeSSE))
+                if ((!doingFloat && (structDesc.IsIntegralSlot(slotCounter))) ||
+                     (doingFloat && (structDesc.IsSseSlot(slotCounter))))
                {


Such an improvement in readability - thanks!

CarolEidt · 2016-01-28T19:06:59Z

Aside from the genGet vs. getGet question above, LGTM

sivarv · 2016-01-28T19:14:15Z

src/inc/corinfo.h

+    // Internal flags - never returned outside of the classification implementation.
+
+    // This value represents a very special type with two eightbytes. 
+    // First ByRef, secont Integer (platform int).


Typo - "second"

Suggestion: "First field is of type ByRef and second field an IntPtr"

LLITCHEV · 2016-01-30T23:32:51Z

@CarolEidt @sivarv PTAL

CarolEidt · 2016-02-01T16:32:31Z

src/jit/codegenxarch.cpp

@@ -1412,6 +1412,221 @@ void CodeGen::genCodeForBinary(GenTree* treeNode)
    genProduceReg(treeNode);
 }

+// Deals with codegen for muti-register struct returns.


These methods lack function headers.

Also, could you add a line not to indicate what has changed? It is difficult to tell now that you have extracted this functionality into a separate method (a good thing to do, but also one of the reasons it is usually prudent to separate refactoring changes from functional changes).

LLITCHEV · 2016-02-01T19:16:02Z

@CarolEidt Good catch. Added function headers and added a comment in the PR description describing the genReturn related refactoring.

sivarv · 2016-02-01T19:35:54Z

src/jit/codegenlinear.h

+    void                getStructReturnRegisters(var_types type0,
+                                                       var_types type1,
+                                                       regNumber* retRegPtr0,
+                                                       regNumber* retRegPtr1);


Nit: formatting of the above 3 params not aligned with the first one.

LLITCHEV · 2016-02-01T23:52:31Z

@CarolEidt @sivarv PTAL the latest changes.

sivarv · 2016-02-01T23:58:15Z

Looks good.

CarolEidt · 2016-02-02T00:22:00Z

src/jit/codegenxarch.cpp

+            getEmitter()->emitIns_R_S(ins_Load(type1), emitTypeSize(type1), retReg1, lclVarPtr->gtLclNum, offset1);
+        }
+
+        // Nothing to do if the op1 of the return statement is a GT_CALL. The call already has the return


Please add a comment here indicating that we are assuming that the registers will not be spilled or moved, and reference the newly created issue that will ensure correct codegen even in the presence of stress modes or other unforeseen changes.

CarolEidt · 2016-02-02T00:23:02Z

LGTM if you add the comment I suggest to refer to issue #2966

It was a long standing assumption in the Jit that a field of a struct can never be of ByRef type. It turns out this assumption is not entirely holding. The System.TypedReference is a very special type that is a struct, defined to have two IntPtr fields. When the VM instantiates this type it changes the type of the first field to ByRef. This change closes a GC hole in the passing this struct by value on the stack. It adds support to the classification algorithm for ByRef references. It also uses the tree types for generating the code that places struct field on the stack by value for passin to a callee. The handling of GT_RETURN in the codegenxarch.cpp has been extracted as a separate routine - genReturn. In case of struct return (multi-register struct return or an implicit retBuf return, it delegates to the genStructReturn method. This is a refactoring as well as making sure that GT_RETFILT is handled properly - the return type of it is always bool or void for the last statement of a finally block. More feedback addressed.

LLITCHEV · 2016-02-02T03:34:39Z

@dotnet-bot OSX x64 Checked Build and Test

LLITCHEV · 2016-02-02T03:40:55Z

@dotnet-bot test OSX x64 Checked Build and Test

LLITCHEV · 2016-02-02T03:56:33Z