Address code review feedback

Author: davidwrighton

src/coreclr/interpreter/compileropt.cpp

@@ -23,6 +23,10 @@ int32_t InterpCompiler::AllocVarOffset(int var, int32_t *pPos)
         if (align > INTERP_STACK_ALIGNMENT)
             align = INTERP_STACK_ALIGNMENT;
     }
+    else
+    {
+        assert(m_pVars[var].interpType != InterpTypeVT || m_compHnd->getClassAlignmentRequirement(m_pVars[var].clsHnd) <= INTERP_STACK_SLOT_SIZE);
+    }
 
     offset = (int32_t)ALIGN_UP_TO(offset, align);
 

src/coreclr/vm/callstubgenerator.cpp

@@ -1405,9 +1405,38 @@ CallStubHeader *CallStubGenerator::GenerateCallStubForSig(MetaSig &sig)
     }
 };
 
-void CallStubGenerator::ComputeCallStub(MetaSig &sig, PCODE *pRoutines)
+enum class RoutineType
+{
+    None,
+    GPReg,
+    FPReg,
+    Stack
+};
+
+void CallStubGenerator::TerminateCurrentRoutineIfNotOfNewType(RoutineType type, PCODE *pRoutines)
 {
+    if ((m_r1 != NoRange) && (type != RoutineType::GPReg))
+    {
+        pRoutines[m_routineIndex++] = GetGPRegRangeRoutine(m_r1, m_r2);
+        m_r1 = NoRange;
+    }
+    else if (m_x1 != NoRange && type != RoutineType::FPReg)
+    {
+        pRoutines[m_routineIndex++] = GetFPRegRangeRoutine(m_x1, m_x2);
+        m_x1 = NoRange;
+    }
+    else if (m_s1 != NoRange && type != RoutineType::Stack)
+    {
+        pRoutines[m_routineIndex++] = GetStackRoutine();
+        pRoutines[m_routineIndex++] = ((int64_t)(m_s2 - m_s1 + 1) << 32) | m_s1;
+        m_s1 = NoRange;
+    }
 
+    return;
+}
+
+void CallStubGenerator::ComputeCallStub(MetaSig &sig, PCODE *pRoutines)
+{
     ArgIterator argIt(&sig);
     int32_t interpreterStackOffset = 0;
 
@@ -1462,7 +1491,7 @@ void CallStubGenerator::ComputeCallStub(MetaSig &sig, PCODE *pRoutines)
 
         // Each entry on the interpreter stack is always aligned to at least 8 bytes, but some arguments are 16 byte aligned
         TypeHandle thArgTypeHandle;
-        if (argIt.GetArgType(&thArgTypeHandle) == ELEMENT_TYPE_VALUETYPE)
+        if ((argIt.GetArgType(&thArgTypeHandle) == ELEMENT_TYPE_VALUETYPE) && thArgTypeHandle.GetSize() > 8)
         {
             unsigned align = CEEInfo::getClassAlignmentRequirementStatic(thArgTypeHandle);
             if (align < INTERP_STACK_SLOT_SIZE)
@@ -1474,24 +1503,9 @@ void CallStubGenerator::ComputeCallStub(MetaSig &sig, PCODE *pRoutines)
                 align = INTERP_STACK_ALIGNMENT;
             }
             assert(align == 8 || align == 16); // At the moment, we can only have an 8 or 16 byte alignment requirement here
-            while (interpreterStackOffset != ALIGN_UP(interpreterStackOffset, align))
+            if (interpreterStackOffset != ALIGN_UP(interpreterStackOffset, align))
             {
-                if (m_r1 != NoRange)
-                {
-                    pRoutines[m_routineIndex++] = GetGPRegRangeRoutine(m_r1, m_r2);
-                    m_r1 = NoRange;
-                }
-                else if (m_x1 != NoRange)
-                {
-                    pRoutines[m_routineIndex++] = GetFPRegRangeRoutine(m_x1, m_x2);
-                    m_x1 = NoRange;
-                }
-                else if (m_s1 != NoRange)
-                {
-                    pRoutines[m_routineIndex++] = GetStackRoutine();
-                    pRoutines[m_routineIndex++] = ((int64_t)(m_s2 - m_s1 + 1) << 32) | m_s1;
-                    m_s1 = NoRange;
-                }
+                TerminateCurrentRoutineIfNotOfNewType(RoutineType::None, pRoutines);
 
                 interpreterStackOffset += INTERP_STACK_SLOT_SIZE;
                 pRoutines[m_routineIndex++] = (PCODE)InjectInterpStackAlign;
@@ -1500,6 +1514,8 @@ void CallStubGenerator::ComputeCallStub(MetaSig &sig, PCODE *pRoutines)
 #endif
             }
 
+            assert(interpreterStackOffset == ALIGN_UP(interpreterStackOffset, align));
+
             interpStackSlotSize = ALIGN_UP(thArgTypeHandle.GetSize(), align);
         }
         interpreterStackOffset += interpStackSlotSize;
@@ -1568,19 +1584,7 @@ void CallStubGenerator::ComputeCallStub(MetaSig &sig, PCODE *pRoutines)
 
     // All arguments were processed, but there is likely a pending ranges to store.
     // Process such a range if any.
-    if (m_r1 != NoRange)
-    {
-        pRoutines[m_routineIndex++] = GetGPRegRangeRoutine(m_r1, m_r2);
-    }
-    else if (m_x1 != NoRange)
-    {
-        pRoutines[m_routineIndex++] = GetFPRegRangeRoutine(m_x1, m_x2);
-    }
-    else if (m_s1 != NoRange)
-    {
-        pRoutines[m_routineIndex++] = GetStackRoutine();
-        pRoutines[m_routineIndex++] = ((int64_t)(m_s2 - m_s1 + 1) << 32) | m_s1;
-    }
+    TerminateCurrentRoutineIfNotOfNewType(RoutineType::None, pRoutines);
 
     ReturnType returnType = GetReturnType(&argIt);
 
@@ -1601,30 +1605,15 @@ void CallStubGenerator::ProcessArgument(ArgIterator *pArgIt, ArgLocDesc& argLocD
 {
     LIMITED_METHOD_CONTRACT;
 
-    // Check if we have a range of registers or stack arguments that we need to store because the current argument
-    // terminates it.
-    if ((argLocDesc.m_cGenReg == 0) && (m_r1 != NoRange))
-    {
-        // No GP register is used to pass the current argument, but we already have a range of GP registers,
-        // store the routine for the range
-        pRoutines[m_routineIndex++] = GetGPRegRangeRoutine(m_r1, m_r2);
-        m_r1 = NoRange;
-    }
-    else if (((argLocDesc.m_cFloatReg == 0)) && (m_x1 != NoRange))
-    {
-        // No floating point register is used to pass the current argument, but we already have a range of FP registers,
-        // store the routine for the range
-        pRoutines[m_routineIndex++] = GetFPRegRangeRoutine(m_x1, m_x2);
-        m_x1 = NoRange;
-    }
-    else if ((argLocDesc.m_byteStackSize == 0) && (m_s1 != NoRange))
-    {
-        // No stack argument is used to pass the current argument, but we already have a range of stack arguments,
-        // store the routine for the range
-        pRoutines[m_routineIndex++] = GetStackRoutine();
-        pRoutines[m_routineIndex++] = ((int64_t)(m_s2 - m_s1 + 1) << 32) | m_s1;
-        m_s1 = NoRange;
-    }
+    RoutineType argType = RoutineType::None;
+    if (argLocDesc.m_cGenReg != 0)
+        argType = RoutineType::GPReg;
+    else if (argLocDesc.m_cFloatReg != 0)
+        argType = RoutineType::FPReg;
+    else if (argLocDesc.m_byteStackSize != 0)
+        argType = RoutineType::Stack;
+    
+    TerminateCurrentRoutineIfNotOfNewType(argType, pRoutines);
 
     if (argLocDesc.m_cGenReg != 0)
     {

src/coreclr/vm/callstubgenerator.h

@@ -149,6 +149,16 @@ class CallStubGenerator
         return sizeof(CallStubHeader) + ((numArgs + 1) * 3 + 1) * sizeof(PCODE);
     }
     void ComputeCallStub(MetaSig &sig, PCODE *pRoutines);
+
+    enum class RoutineType
+    {
+        None,
+        GPReg,
+        FPReg,
+        Stack
+    };
+
+    void TerminateCurrentRoutineIfNotOfNewType(RoutineType type, PCODE *pRoutines);
 };
 
 void InitCallStubGenerator();