Fix genCodeForIndexAddr

This does some weird things - treats the array length as 64 bit when it's in fact 32 bit, fails to zero extend TYP_INT indices, creates new GT_IND/GT_LEA nodes out of thin air.

diff --git a/src/jit/codegenxarch.cpp b/src/jit/codegenxarch.cpp
index ced48c2..aa701be 100644 (file)
--- a/src/jit/codegenxarch.cpp
+++ b/src/jit/codegenxarch.cpp
@@ -4607,79 +4607,87 @@ void CodeGen::genCodeForIndexAddr(GenTreeIndexAddr* node)
     GenTree* const base  = node->Arr();
     GenTree* const index = node->Index();
 
-    genConsumeReg(base);
-    genConsumeReg(index);
+    const regNumber baseReg  = genConsumeReg(base);
+    const regNumber indexReg = genConsumeReg(index);
+    const regNumber dstReg   = node->gtRegNum;
 
     // NOTE: `genConsumeReg` marks the consumed register as not a GC pointer, as it assumes that the input registers
     // die at the first instruction generated by the node. This is not the case for `INDEX_ADDR`, however, as the
     // base register is multiply-used. As such, we need to mark the base register as containing a GC pointer until
     // we are finished generating the code for this node.
 
-    gcInfo.gcMarkRegPtrVal(base->gtRegNum, base->TypeGet());
-    assert(!varTypeIsGC(index->TypeGet()));
+    gcInfo.gcMarkRegPtrVal(baseReg, base->TypeGet());
+    assert(varTypeIsIntegral(index->TypeGet()));
 
     regNumber tmpReg = REG_NA;
 
     // Generate the bounds check if necessary.
     if ((node->gtFlags & GTF_INX_RNGCHK) != 0)
     {
-        // Create a GT_IND(GT_LEA)) tree for the array length access.
-        GenTreeAddrMode arrLenAddr(base->TypeGet(), base, nullptr, 0, node->gtLenOffset);
-        arrLenAddr.gtRegNum = REG_NA;
-        arrLenAddr.SetContained();
-
-        GenTreeIndir arrLen = indirForm(TYP_INT, &arrLenAddr);
-
 #ifdef _TARGET_64BIT_
         // The CLI Spec allows an array to be indexed by either an int32 or a native int.  In the case that the index
-        // is a native int on a 64-bit platform, we will need to widen the array length and the compare.
+        // is a native int on a 64-bit platform, we will need to widen the array length and then compare.
         if (index->TypeGet() == TYP_I_IMPL)
         {
-            // Load the array length into a register.
-            tmpReg          = node->GetSingleTempReg();
-            arrLen.gtRegNum = tmpReg;
-            arrLen.ClearContained();
-            getEmitter()->emitInsLoadInd(ins_Load(TYP_INT), EA_4BYTE, arrLen.gtRegNum, &arrLen);
+            tmpReg = node->GetSingleTempReg();
+            getEmitter()->emitIns_R_AR(INS_mov, EA_4BYTE, tmpReg, baseReg, static_cast<int>(node->gtLenOffset));
+            getEmitter()->emitIns_R_R(INS_cmp, EA_8BYTE, indexReg, tmpReg);
         }
         else
-#endif
+#endif // _TARGET_64BIT_
         {
-            assert(varTypeIsIntegral(index->TypeGet()));
-
-            arrLen.gtRegNum = REG_NA;
-            arrLen.SetContained();
+            getEmitter()->emitIns_R_AR(INS_cmp, EA_4BYTE, indexReg, baseReg, static_cast<int>(node->gtLenOffset));
         }
 
-        // Generate the range check.
-        getEmitter()->emitInsBinary(INS_cmp, emitTypeSize(TYP_I_IMPL), index, &arrLen);
         genJumpToThrowHlpBlk(EJ_jae, SCK_RNGCHK_FAIL, node->gtIndRngFailBB);
     }
 
+#ifdef _TARGET_64BIT_
+    if (index->TypeGet() != TYP_I_IMPL)
+    {
+        // LEA needs 64-bit operands so we need to widen the index if it's TYP_INT.
+        // Since it's TYP_INT the upper 32 bits aren't used so we should be able
+        // to widen in place, without needing a temporary register.
+        getEmitter()->emitIns_R_R(INS_mov, EA_4BYTE, indexReg, indexReg);
+    }
+#endif // _TARGET_64BIT_
+
     // Compute the address of the array element.
-    switch (node->gtElemSize)
+    unsigned scale = node->gtElemSize;
+
+    switch (scale)
     {
         case 1:
         case 2:
         case 4:
         case 8:
-            getEmitter()->emitIns_R_ARX(INS_lea, emitTypeSize(node), node->gtRegNum, base->gtRegNum, index->gtRegNum,
-                                        node->gtElemSize, static_cast<int>(node->gtElemOffset));
+            tmpReg = indexReg;
             break;
 
         default:
-        {
-            // Multiply the index by the element size.
-            //
-            // TODO-CQ: this should really just use `imul index, index, #gtElemSize`
-            tmpReg = (tmpReg == REG_NA) ? node->GetSingleTempReg() : tmpReg;
-            CodeGen::genSetRegToIcon(tmpReg, (ssize_t)node->gtElemSize, TYP_INT);
-            inst_RV_RV(INS_imul, tmpReg, index->gtRegNum);
-            getEmitter()->emitIns_R_ARX(INS_lea, emitTypeSize(node), node->gtRegNum, base->gtRegNum, tmpReg, 1,
-                                        static_cast<int>(node->gtElemOffset));
+            if (tmpReg == REG_NA)
+            {
+                tmpReg = node->GetSingleTempReg();
+            }
+#ifdef _TARGET_64BIT_
+            if (scale > INT32_MAX)
+            {
+                genSetRegToIcon(tmpReg, static_cast<ssize_t>(scale), TYP_INT);
+                getEmitter()->emitIns_R_R(INS_imul, EA_PTRSIZE, tmpReg, indexReg);
+            }
+            else
+#endif // _TARGET_64BIT_
+            {
+                getEmitter()->emitIns_R_I(emitter::inst3opImulForReg(tmpReg), EA_PTRSIZE, indexReg,
+                                          static_cast<ssize_t>(scale));
+            }
+            scale = 1;
             break;
-        }
     }
 
+    getEmitter()->emitIns_R_ARX(INS_lea, emitTypeSize(node->TypeGet()), dstReg, baseReg, tmpReg, scale,
+                                static_cast<int>(node->gtElemOffset));
+
     gcInfo.gcMarkRegSetNpt(base->gtGetRegMask());
 
     genProduceReg(node);