resolve comments.

dotnet · Dec 16, 2024 · 5d3cca2 · 5d3cca2
1 parent 6502ae1
commit 5d3cca2
Show file tree

Hide file tree

Showing 3 changed files with 42 additions and 26 deletions.
diff --git a/src/coreclr/jit/codegenxarch.cpp b/src/coreclr/jit/codegenxarch.cpp
@@ -9064,7 +9064,10 @@ void CodeGen::genAmd64EmitterUnitTestsApx()
     genDefineTempLabel(genCreateTempLabel());
 
     // This test suite needs REX2 enabled.
-    assert(theEmitter->UseRex2Encoding() || theEmitter->emitComp->DoJitStressRex2Encoding());
+    if (!theEmitter->UseRex2Encoding() && !theEmitter->emitComp->DoJitStressRex2Encoding())
+    {
+        return;
+    }
 
     theEmitter->emitIns_R_R(INS_add, EA_1BYTE, REG_EAX, REG_ECX);
     theEmitter->emitIns_R_R(INS_add, EA_2BYTE, REG_EAX, REG_ECX);
@@ -9205,18 +9208,12 @@ void CodeGen::genAmd64EmitterUnitTestsApx()
     theEmitter->emitIns_R(INS_div, EA_8BYTE, REG_EDX);
     theEmitter->emitIns_R(INS_mulEAX, EA_8BYTE, REG_EDX);
 
-    // Note:
-    // All the tests below rely on the runtime status of the stack this unit tests attaching to,
-    // it might fail due to stack value unavailable/mismatch, since these tests are mainly for
-    // encoding correctness check, this kind of failures may be considered as not harmful.
-
-    GenTree* stkNum = theEmitter->emitComp->stackState.esStack[0].val;
-    stkNum->SetRegNum(REG_EDX);
-    stkNum->SetUnusedValue();
-    GenTreeIndir load = indirForm(TYP_INT, stkNum);
+    GenTreePhysReg physReg(REG_EDX);
+    physReg.SetRegNum(REG_EDX);
+    GenTreeIndir load = indirForm(TYP_INT, &physReg);
 
-    theEmitter->emitIns_R_A(INS_add, EA_4BYTE, REG_EAX, &load);
-    theEmitter->emitIns_R_A(INS_add, EA_4BYTE, REG_EAX, &load);
+    theEmitter->emitIns_R_A(INS_add, EA_1BYTE, REG_EAX, &load);
+    theEmitter->emitIns_R_A(INS_add, EA_2BYTE, REG_EAX, &load);
     theEmitter->emitIns_R_A(INS_add, EA_4BYTE, REG_EAX, &load);
     theEmitter->emitIns_R_A(INS_add, EA_8BYTE, REG_EAX, &load);
     theEmitter->emitIns_R_A(INS_or, EA_4BYTE, REG_EAX, &load);
@@ -9230,6 +9227,11 @@ void CodeGen::genAmd64EmitterUnitTestsApx()
     theEmitter->emitIns_R_A(INS_bsf, EA_4BYTE, REG_EAX, &load);
     theEmitter->emitIns_R_A(INS_bsr, EA_4BYTE, REG_EAX, &load);
 
+    // Note:
+    // All the tests below rely on the runtime status of the stack this unit tests attaching to,
+    // it might fail due to stack value unavailable/mismatch, since these tests are mainly for
+    // encoding correctness check, this kind of failures may be considered as not harmful.
+
     theEmitter->emitIns_R_S(INS_add, EA_1BYTE, REG_EAX, 0, 0);
     theEmitter->emitIns_R_S(INS_add, EA_2BYTE, REG_EAX, 0, 0);
     theEmitter->emitIns_R_S(INS_add, EA_4BYTE, REG_EAX, 0, 0);

diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h
@@ -9970,7 +9970,7 @@ class Compiler
         // otherwise use VEX encoding but can be EVEX encoded to use EVEX encoding
         // This requires AVX512F, AVX512BW, AVX512CD, AVX512DQ, and AVX512VL support
 
-        if (JitConfig.JitStressEvexEncoding() && IsBaselineVector512IsaSupportedOpportunistically())
+        if (JitStressEvexEncoding() && IsBaselineVector512IsaSupportedOpportunistically())
         {
             assert(compIsaSupportedDebugOnly(InstructionSet_AVX512F));
             assert(compIsaSupportedDebugOnly(InstructionSet_AVX512F_VL));
@@ -9983,7 +9983,7 @@ class Compiler
 
             return true;
         }
-        else if (JitConfig.JitStressEvexEncoding() && compOpportunisticallyDependsOn(InstructionSet_AVX10v1))
+        else if (JitStressEvexEncoding() && compOpportunisticallyDependsOn(InstructionSet_AVX10v1))
         {
             return true;
         }
@@ -10005,13 +10005,27 @@ class Compiler
         {
             // we should make sure EVEX is also stressed when REX2 is stressed, as we will need to guarantee EGPR
             // functionality is properly turned on for every instructions when REX2 is stress.
-            assert(JitConfig.JitStressEvexEncoding());
             return true;
         }
 #endif // DEBUG
 
         return false;
     }
+
+    //------------------------------------------------------------------------
+    // JitStressEvexEncoding- Answer the question: Is Evex stress knob set
+    //
+    // Returns:
+    //    `true` if user requests REX2 encoding.
+    //
+    bool JitStressEvexEncoding() const
+    {
+#ifdef DEBUG
+        return JitConfig.JitStressEvexEncoding() || JitConfig.JitStressRex2Encoding();
+#endif // DEBUG
+
+        return false;
+    }
 #endif // TARGET_XARCH
 
     /*

diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp
@@ -304,15 +304,15 @@ bool emitter::IsLegacyMap1(code_t code) const
         // 2-byte
         return true;
     }
-    if ((code & 0xFF0000) == 0x0F0000)
+    if ((code & 0xFFFF0000) == 0x000F0000)
     {
         // 3-byte
         return true;
     }
 
-    if ((code & 0xFF000000) == 0x0F000000)
+    if ((code & 0xFF00FF00) == 0x0F000000)
     {
-        // 4-byte, need to check if PP is prefixs
+        // 4-byte, need to check if PP is a prefix.
         BYTE prefix = (BYTE)((code & 0xFF0000) >> 16);
         return ((prefix == 0xF2) || (prefix == 0xF3) || (prefix == 0x66));
     }
@@ -1369,7 +1369,7 @@ bool emitter::TakesRex2Prefix(const instrDesc* id) const
     // TODO-xarch-apx:
     // At this stage, we are only using REX2 in the case that non-simd integer instructions
     // with EGPRs being used in its operands, it could be either direct register uses, or
-    // memory addresssig operands, i.e. index and base.
+    // memory addressing operands, i.e. index and base.
     instruction ins = id->idIns();
     if (!IsRex2EncodableInstruction(ins))
     {
@@ -1381,17 +1381,17 @@ bool emitter::TakesRex2Prefix(const instrDesc* id) const
         return false;
     }
 
-#if defined(DEBUG)
-    if (emitComp->DoJitStressRex2Encoding())
+    if (HasExtendedGPReg(id))
     {
         return true;
     }
-#endif // DEBUG
 
-    if (HasExtendedGPReg(id))
+#if defined(DEBUG)
+    if (emitComp->DoJitStressRex2Encoding())
     {
         return true;
     }
+#endif // DEBUG
 
     return false;
 }
@@ -1786,7 +1786,7 @@ bool emitter::HasHighSIMDReg(const instrDesc* id) const
 }
 
 //------------------------------------------------------------------------
-// HasExtendedGPReg: Checks if an instruction uses a extended general purpose registers - EGPRs (r16-r31)
+// HasExtendedGPReg: Checks if an instruction uses an extended general-purpose register - EGPR (r16-r31)
 // and will require one of the REX2 EGPR bits (REX2.R4/R3, REX2.B4/B3, REX2.X4/X3)
 //
 // Arguments:
@@ -2571,7 +2571,7 @@ unsigned emitter::emitOutputRexOrSimdPrefixIfNeeded(instruction ins, BYTE* dst,
         if ((code & 0xFF) == 0x0F)
         {
             // some map-1 instructions have opcode in forms like:
-            // XX0F, remove the leading 0x0F byte as it have been recoreded in REX2.
+            // XX0F, remove the leading 0x0F byte as it has been recorded in REX2.
             code = code >> 8;
         }
 
@@ -3556,7 +3556,7 @@ inline unsigned emitter::insEncodeReg012(const instrDesc* id, regNumber reg, emi
         }
         if (false /*reg >= REG_R16 && reg <= REG_R31*/)
         {
-            // seperate the encoding for REX2.B3/B4, REX2.B3 will be handled in `AddRexBPrefix`.
+            // Seperate the encoding for REX2.B3/B4, REX2.B3 will be handled in `AddRexBPrefix`.
             assert(TakesRex2Prefix(id));
             *code |= 0x001000000000ULL; // REX2.B4
         }