Delete JitDoOldStructRetyping artifacts.

[sandreenko]

Delete the code that was handling JitDoOldStructRetyping = true.

[sandreenko]

/azp run runtime-coreclr jitstress

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[sandreenko]

PTAL @dotnet/jit-contrib

[AndyAyersMS]

Nice to see all that code going away.

[briansull]

This is is a possible change in the functionality of this method before we were handling SPK_EnclosingType, now we don't.

[sandreenko]

SPK_EnclosingType becomes unreachable without compDoOldStructRetyping, I have added an assert that only SPK_ByValueAsHfa and SPK_ByValue can come here.

We used SPK_EnclosingType only for structs that are smaller than TARGET_POINTER_SIZE and we were retyping them as int/long, now we keep them as structs, so now need for this part.

The part that is left handles the multi-reg case.

[briansull]

Your change here also removes this folding,
This doesn't seem to be directly related to compDoOldStructRetyping

[sandreenko]

It is also unreachable, the previous condition was:

if (!compDoOldStructRetyping() && (!op->IsCall() || !op->AsCall()->TreatAsHasRetBufArg(this)))

now it is

if (!op->IsCall() || !op->AsCall()->TreatAsHasRetBufArg(this))

so op can't be anything but call.

[AndyAyersMS]

Is the SPK_EnclosingType clause in fgUpdateInlineReturnExpressionPlaceHolder also now unreachable? Would be nice if it was.

I pushed the change to delete the handling, there are no asm diffs but Jit log is smaller now.

Excellent. Thanks.

[AndyAyersMS]

Is the SPK_EnclosingType clause in fgUpdateInlineReturnExpressionPlaceHolder also now unreachable? Would be nice if it was.

runtime/src/coreclr/jit/fginline.cpp

Lines 620 to 666 in c5d16e0

	case SPK_EnclosingType:
	{
	// For enclosing type returns, we must return the call value to a temp since
	// the return type is larger than the struct type.
	if (!tree->IsCall())
	{
	break;
	}
	GenTreeCall* call = tree->AsCall();
	assert(call->gtReturnType == TYP_STRUCT);
	if (call->gtReturnType != TYP_STRUCT)
	{
	break;
	}
	JITDUMP("\nCall returns small struct via enclosing type, retyping. Before:\n");
	DISPTREE(call);
	// Create new struct typed temp for return value
	const unsigned tmpNum =
	comp->lvaGrabTemp(true DEBUGARG("small struct return temp for rejected inline"));
	comp->lvaSetStruct(tmpNum, retClsHnd, false);
	GenTree* assign = comp->gtNewTempAssign(tmpNum, call);
	// Modify assign tree and call return types to the primitive return type
	call->gtReturnType = returnType;
	call->gtType = returnType;
	assign->gtType = returnType;
	// Modify the temp reference in the assign as a primitive reference via GT_LCL_FLD
	GenTree* tempAsPrimitive = assign->AsOp()->gtOp1;
	assert(tempAsPrimitive->gtOper == GT_LCL_VAR);
	tempAsPrimitive->gtType = returnType;
	tempAsPrimitive->ChangeOper(GT_LCL_FLD);
	// Return temp as value of call tree via comma
	GenTree* tempAsStruct = comp->gtNewLclvNode(tmpNum, TYP_STRUCT);
	GenTree* comma = comp->gtNewOperNode(GT_COMMA, TYP_STRUCT, assign, tempAsStruct);
	parent->ReplaceOperand(pTree, comma);
	JITDUMP("\nAfter:\n");
	DISPTREE(comma);
	}
	break;

[sandreenko]

Is the SPK_EnclosingType clause in fgUpdateInlineReturnExpressionPlaceHolder also now unreachable? Would be nice if it was.

It is reachable but we don't need to do anything for it,

runtime/src/coreclr/jit/fginline.cpp

Lines 577 to 584 in f33c440

	// If an inline was rejected and the call returns a struct, we may
	// have deferred some work when importing call for cases where the
	// struct is returned in register(s).
	//
	// See the bail-out clauses in impFixupCallStructReturn for inline
	// candidates.
	//
	// Do the deferred work now.

we don't do retyping so there is no deferred work nowadays.

I pushed the change to delete the handling, there are no asm diffs but Jit log is smaller now.

For example, for

    [StructLayout(LayoutKind.Explicit, Size = 3)]
    public struct ThreeByteStruct
    {
        [FieldOffset(0)] public Byte b1;
        [FieldOffset(0)] public Byte b2;
        [FieldOffset(0)] public Byte b3;
    }


    static ThreeByteStruct Test2()
    {
        ThreeByteStruct s = new ThreeByteStruct();
        s.b1 = 1;
        s.b2 = 2;
        s.b3 = 3;
        return s;
    }

on unix x64 we have:

               [000004] -ACXG+------              *  ASG       struct (copy)
               [000002] D----+-N----              +--*  LCL_VAR   struct<ThreeByteStruct, 3> V01 tmp1         
               [000000] --CXG+------              \--*  CALL      struct TestMethodInfo.Tes

instead of

               [000004] -ACXG+------              *  ASG       struct (copy)
               [000002] D----+-N----              +--*  LCL_VAR   struct<ThreeByteStruct, 3> V01 tmp1         
               [000017] -ACXG+------              \--*  IND       struct
               [000015] -ACXG+------                 \--*  COMMA     byref 
               [000013] -ACXG+------                    +--*  ASG       int   
               [000011] U----+-N----                    |  +--*  LCL_FLD   int    V02 tmp2         [+0]
               [000000] --CXG+-N----                    |  \--*  CALL      int    TestMethodInfo.Test
               [000016] -----+------                    \--*  ADDR      byref 
               [000014] -----+-N----                       \--*  LCL_VAR   struct<ThreeByteStruct, 3> V02 tmp2 

[briansull]

Looks Good