Review response.

src/coreclr/src/jit/compiler.h

@@ -7974,7 +7974,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
     // vector type (i.e. do not analyze or promote its fields).
     // Note that all but the fixed vector types are opaque, even though they may
     // actually be declared as having fields.
-    bool isOpaqueSIMDType(CORINFO_CLASS_HANDLE structHandle)
+    bool isOpaqueSIMDType(CORINFO_CLASS_HANDLE structHandle) const
     {
         return ((m_simdHandleCache != nullptr) && (structHandle != m_simdHandleCache->SIMDVector2Handle) &&
                 (structHandle != m_simdHandleCache->SIMDVector3Handle) &&
@@ -7990,7 +7990,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
     }
 
     // Returns true if the lclVar is an opaque SIMD type.
-    bool isOpaqueSIMDLclVar(LclVarDsc* varDsc)
+    bool isOpaqueSIMDLclVar(const LclVarDsc* varDsc) const
     {
         if (!varDsc->lvSIMDType)
         {

src/coreclr/src/jit/lclvars.cpp

@@ -3680,13 +3680,19 @@ bool LclVarDsc::CanBeReplacedWithItsField(Compiler* comp) const
     }
 
 #if defined(FEATURE_SIMD)
+    assert(!comp->isOpaqueSIMDLclVar(this)); // opaque SIMD can't be promoted.
     // If we return `struct A { SIMD16 a; }` we split the struct into several fields.
-    // In order to do that we have to have its field in memory. Right now lowering cannot
+    // In order to do that we have to have its field `a` in memory. Right now lowering cannot
     // handle RETURN struct(multiple registers)->SIMD16(one register), but it can be improved.
     LclVarDsc* fieldDsc = comp->lvaGetDesc(lvFieldLclStart);
     if (fieldDsc->TypeGet() == TYP_SIMD12 || fieldDsc->TypeGet() == TYP_SIMD16)
     {
+#if defined(TARGET_ARM64)
         return false;
+#else  // !TARGET_ARM64
+        // TODO-X64-CQ: it will be reachable after https://github.com/dotnet/runtime/issues/9578.
+        unreached();
+#endif //! TARGET_ARM64
     }
 #endif // FEATURE_SIMD
 

src/coreclr/src/jit/lower.cpp

@@ -245,7 +245,7 @@ GenTree* Lowering::LowerNode(GenTree* node)
             if (node->AsBlk()->Data()->IsCall())
             {
                 assert(!comp->compDoOldStructRetyping());
-                LowerStoreCallStruct(node->AsBlk());
+                LowerStoreSingleRegCallStruct(node->AsBlk());
                 break;
             }
             __fallthrough;
@@ -2939,12 +2939,30 @@ void Lowering::LowerRet(GenTreeUnOp* ret)
     JITDUMP("============");
 
     GenTree* retVal = ret->gtGetOp1();
-    bool     needBitcast =
+    // There are two kinds of retyping:
+    // - A simple bitcast can be inserted when:
+    //   - We're returning a floating type as an integral type or vice-versa, or
+    //   - We're returning a struct as a primitive type and using the old form of retyping.
+    // - If we're returning a struct as a primitive type and *not* using old retying, we change the type of
+    //   'retval' in 'LowerRetStructLclVar()'
+    bool needBitcast =
         (ret->TypeGet() != TYP_VOID) && (varTypeUsesFloatReg(ret) != varTypeUsesFloatReg(ret->gtGetOp1()));
-    if (needBitcast && !varTypeIsStruct(ret) && (!varTypeIsStruct(retVal) || comp->compDoOldStructRetyping()))
+    if (needBitcast && ((!varTypeIsStruct(ret) && !varTypeIsStruct(retVal)) || comp->compDoOldStructRetyping()))
     {
-        // Add a simple bitcast for an old retyping or when both types are not structs.
-        // If one type is a struct it will be handled below for !compDoOldStructRetyping.
+// Add a simple bitcast for an old retyping or when both types are not structs.
+// If one type is a struct it will be handled below for !compDoOldStructRetyping.
+#if defined(DEBUG)
+        if (comp->compDoOldStructRetyping())
+        {
+            assert(varTypeIsSIMD(ret) || !!varTypeIsStruct(ret));
+            assert(varTypeIsSIMD(retVal) || !!varTypeIsStruct(retVal));
+        }
+        else
+        {
+            assert(!varTypeIsStruct(ret) && !varTypeIsStruct(retVal));
+        }
+#endif
+
         GenTree* bitcast = comp->gtNewBitCastNode(ret->TypeGet(), retVal);
         ret->gtOp1       = bitcast;
         BlockRange().InsertBefore(ret, bitcast);
@@ -2973,22 +2991,15 @@ void Lowering::LowerRet(GenTreeUnOp* ret)
                 assert(!comp->compDoOldStructRetyping());
                 assert(comp->info.compRetNativeType != TYP_STRUCT);
 
-                bool      constStructInit  = retVal->IsConstInitVal();
-                bool      actualTypesMatch = false;
                 var_types retActualType    = genActualType(comp->info.compRetNativeType);
                 var_types retValActualType = genActualType(retVal->TypeGet());
-                if (retActualType == retValActualType)
-                {
-                    // This could happen if we have retyped op1 as a primitive type during struct promotion,
-                    // check `retypedFieldsMap` for details.
-                    actualTypesMatch = true;
-                }
 
-                bool implicitCastFromSameOrBiggerSize = false;
-                if (genTypeSize(retActualType) <= genTypeSize(retValActualType))
-                {
-                    implicitCastFromSameOrBiggerSize = true;
-                }
+                bool constStructInit                  = retVal->IsConstInitVal();
+                bool implicitCastFromSameOrBiggerSize = (genTypeSize(retActualType) <= genTypeSize(retValActualType));
+
+                // This could happen if we have retyped op1 as a primitive type during struct promotion,
+                // check `retypedFieldsMap` for details.
+                bool actualTypesMatch = (retActualType == retValActualType);
 
                 assert(actualTypesMatch || constStructInit || implicitCastFromSameOrBiggerSize);
             }
@@ -3015,7 +3026,7 @@ void Lowering::LowerRet(GenTreeUnOp* ret)
                 // Return struct as a primitive using Unsafe cast.
                 assert(!comp->compDoOldStructRetyping());
                 assert(retVal->OperIs(GT_LCL_VAR));
-                LowerRetStructLclVar(ret);
+                LowerRetSingleRegStructLclVar(ret);
             }
         }
     }
@@ -3048,7 +3059,7 @@ void Lowering::LowerStoreLocCommon(GenTreeLclVarCommon* lclStore)
 
     if (!dstIsMultiReg && varTypeIsStruct(varDsc))
     {
-        // TODO: we want to check `varDsc->lvRegStruct` as the last condition instead of `!varDsc->lvPromoted`,
+        // TODO-Cleanup: we want to check `varDsc->lvRegStruct` as the last condition instead of `!varDsc->lvPromoted`,
         // but we do not set it for `CSE` vars so it is currently failing.
         assert(varDsc->CanBeReplacedWithItsField(comp) || varDsc->lvDoNotEnregister || !varDsc->lvPromoted);
         if (varDsc->CanBeReplacedWithItsField(comp))
@@ -3094,8 +3105,7 @@ void Lowering::LowerStoreLocCommon(GenTreeLclVarCommon* lclStore)
         }
         CheckMultiRegLclVar(lclStore->AsLclVar(), retTypeDesc);
     }
-    if ((lclStore->TypeGet() == TYP_STRUCT) && !srcIsMultiReg && varTypeIsStruct(lclStore->TypeGet()) &&
-        (src->OperGet() != GT_PHI))
+    if ((lclStore->TypeGet() == TYP_STRUCT) && !srcIsMultiReg && (src->OperGet() != GT_PHI))
     {
         if (src->OperGet() == GT_CALL)
         {
@@ -3210,7 +3220,7 @@ void Lowering::LowerRetStruct(GenTreeUnOp* ret)
             {
                 assert(retVal->OperIs(GT_LCL_VAR));
                 assert(!comp->compDoOldStructRetyping());
-                LowerRetStructLclVar(ret);
+                LowerRetSingleRegStructLclVar(ret);
             }
             return;
         }
@@ -3260,7 +3270,7 @@ void Lowering::LowerRetStruct(GenTreeUnOp* ret)
             break;
 
         case GT_LCL_VAR:
-            LowerRetStructLclVar(ret);
+            LowerRetSingleRegStructLclVar(ret);
             break;
 
 #if defined(FEATURE_SIMD) || defined(FEATURE_HW_INTRINSICS)
@@ -3297,8 +3307,10 @@ void Lowering::LowerRetStruct(GenTreeUnOp* ret)
 
 #if defined(TARGET_AMD64) || defined(TARGET_ARM64) || defined(TARGET_ARM)
         case GT_CNS_DBL:
-            // Currently we are not promoting structs with a single float field.
-            // TODO: can improve `GT_CNS_DBL` handling for supported platforms, but
+            // Currently we are not promoting structs with a single float field,
+            // https://github.com/dotnet/runtime/issues/4323
+
+            // TODO-CQ: can improve `GT_CNS_DBL` handling for supported platforms, but
             // because it is only x86 nowadays it is not worth it.
             unreached();
 #endif
@@ -3317,7 +3329,7 @@ void Lowering::LowerRetStruct(GenTreeUnOp* ret)
 }
 
 //----------------------------------------------------------------------------------------------
-// LowerRetStructLclVar: Lowers a return node with a struct lclVar as a source.
+// LowerRetSingleRegStructLclVar: Lowers a return node with a struct lclVar as a source.
 //
 // Arguments:
 //    node - The return node to lower.
@@ -3327,7 +3339,7 @@ void Lowering::LowerRetStruct(GenTreeUnOp* ret)
 //    - if LclVar is allocated in memory then read it as return type;
 //    - if LclVar can be enregistered read it as register type and add a bitcast if necessary;
 //
-void Lowering::LowerRetStructLclVar(GenTreeUnOp* ret)
+void Lowering::LowerRetSingleRegStructLclVar(GenTreeUnOp* ret)
 {
     assert(!comp->compMethodReturnsMultiRegRegTypeAlternate());
     assert(!comp->compDoOldStructRetyping());
@@ -3342,6 +3354,7 @@ void Lowering::LowerRetStructLclVar(GenTreeUnOp* ret)
         // We can replace the struct with its only field and keep the field on a register.
         unsigned   fieldLclNum = varDsc->lvFieldLclStart;
         LclVarDsc* fieldDsc    = comp->lvaGetDesc(fieldLclNum);
+        assert(varTypeIsSmallInt(fieldDsc->lvType)); // For a non-small type it had to be done in morph.
 
         lclVar->SetLclNum(fieldLclNum);
         JITDUMP("Replacing an independently promoted local var V%02u with its only field  V%02u for the return "
@@ -3469,20 +3482,22 @@ void Lowering::LowerCallStruct(GenTreeCall* call)
 }
 
 //----------------------------------------------------------------------------------------------
-// LowerStoreCallStruct: Lowers a store block where source is a struct typed call.
+// LowerStoreSingleRegCallStruct: Lowers a store block where the source is a struct typed call.
 //
 // Arguments:
 //     store - The store node to lower.
 //
 // Notes:
-//    - it spills the call's result if it can be retyped as a primitive type.
+//    - the function is only for calls that return one register;
+//    - it spills the call's result if it can be retyped as a primitive type;
 //
-void Lowering::LowerStoreCallStruct(GenTreeBlk* store)
+void Lowering::LowerStoreSingleRegCallStruct(GenTreeBlk* store)
 {
     assert(!comp->compDoOldStructRetyping());
     assert(varTypeIsStruct(store));
     assert(store->Data()->IsCall());
     GenTreeCall* call = store->Data()->AsCall();
+    assert(!call->HasMultiRegRetVal());
 
     const ClassLayout* layout  = store->GetLayout();
     const var_types    regType = layout->GetRegisterType();

src/coreclr/src/jit/lower.h

@@ -134,9 +134,9 @@ class Lowering final : public Phase
     void LowerRet(GenTreeUnOp* ret);
     void LowerStoreLocCommon(GenTreeLclVarCommon* lclVar);
     void LowerRetStruct(GenTreeUnOp* ret);
-    void LowerRetStructLclVar(GenTreeUnOp* ret);
+    void LowerRetSingleRegStructLclVar(GenTreeUnOp* ret);
     void LowerCallStruct(GenTreeCall* call);
-    void LowerStoreCallStruct(GenTreeBlk* store);
+    void LowerStoreSingleRegCallStruct(GenTreeBlk* store);
 #if !defined(WINDOWS_AMD64_ABI)
     GenTreeLclVar* SpillStructCallResult(GenTreeCall* call) const;
 #endif // WINDOWS_AMD64_ABI

src/coreclr/src/jit/morph.cpp

@@ -11985,30 +11985,36 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
                 {
                     op1 = fgMorphRetInd(tree->AsUnOp());
                 }
-                if (op1->OperIs(GT_LCL_VAR) && (genReturnBB == nullptr))
+                if (op1->OperIs(GT_LCL_VAR))
                 {
-                    // With a `genReturnBB` it will be done via field by field asignment.
+                    // With a `genReturnBB` this `RETURN(src)` tree will be replaced by a `ASG(genReturnLocal, src)`
+                    // and `ASG` will be tranformed into field by field copy without parent local referencing if
+                    // possible.
                     GenTreeLclVar* lclVar = op1->AsLclVar();
-                    LclVarDsc*     varDsc = lvaGetDesc(lclVar);
-                    if (varDsc->CanBeReplacedWithItsField(this))
+                    unsigned       lclNum = lclVar->GetLclNum();
+                    if ((genReturnLocal == BAD_VAR_NUM) || (genReturnLocal == lclNum))
                     {
-                        // We can replace the struct with its only field and allow copy propogation to replace
-                        // return value that was written as a field.
-                        unsigned   fieldLclNum = varDsc->lvFieldLclStart;
-                        LclVarDsc* fieldDsc    = lvaGetDesc(fieldLclNum);
-
-                        if (!varTypeIsSmallInt(fieldDsc->lvType))
+                        LclVarDsc* varDsc = lvaGetDesc(lclVar);
+                        if (varDsc->CanBeReplacedWithItsField(this))
                         {
-                            // TODO: support that substitution for small types without creating `CAST` node.
-                            // When a small struct is returned in a register higher bits could be left in undefined
-                            // state.
-                            JITDUMP(
-                                "Replacing an independently promoted local var V%02u with its only field  V%02u for "
-                                "the return [%06u]\n",
-                                lclVar->GetLclNum(), fieldLclNum, dspTreeID(tree));
-                            lclVar->SetLclNum(fieldLclNum);
-                            var_types fieldType = fieldDsc->lvType;
-                            lclVar->ChangeType(fieldDsc->lvType);
+                            // We can replace the struct with its only field and allow copy propogation to replace
+                            // return value that was written as a field.
+                            unsigned   fieldLclNum = varDsc->lvFieldLclStart;
+                            LclVarDsc* fieldDsc    = lvaGetDesc(fieldLclNum);
+
+                            if (!varTypeIsSmallInt(fieldDsc->lvType))
+                            {
+                                // TODO-CQ: support that substitution for small types without creating `CAST` node.
+                                // When a small struct is returned in a register higher bits could be left in undefined
+                                // state.
+                                JITDUMP("Replacing an independently promoted local var V%02u with its only field  "
+                                        "V%02u for "
+                                        "the return [%06u]\n",
+                                        lclVar->GetLclNum(), fieldLclNum, dspTreeID(tree));
+                                lclVar->SetLclNum(fieldLclNum);
+                                var_types fieldType = fieldDsc->lvType;
+                                lclVar->ChangeType(fieldDsc->lvType);
+                            }
                         }
                     }
                 }