Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Use a smaller expansion of GT_INDEX in MinOpts. #13245

Merged
pgavlin merged 2 commits into from
Aug 25, 2017
Merged

Use a smaller expansion of GT_INDEX in MinOpts. #13245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cdec3b5
Use a smaller expansion of `GT_INDEX` in MinOpts.
pgavlin Aug 5, 2017
71b9152
Address PR feedback.
pgavlin Aug 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 89 additions & 0 deletions src/jit/codegenarmarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,10 @@ void CodeGen::genCodeForTreeNode(GenTreePtr treeNode)
genLeaInstruction(treeNode->AsAddrMode());
break;

case GT_INDEX_ADDR:
genCodeForIndexAddr(treeNode->AsIndexAddr());
break;

case GT_IND:
genCodeForIndir(treeNode->AsIndir());
break;
Expand Down Expand Up @@ -1539,6 +1543,91 @@ void CodeGen::genCodeForLclFld(GenTreeLclFld* tree)
genProduceReg(tree);
}

//------------------------------------------------------------------------
// genCodeForIndexAddr: Produce code for a GT_INDEX_ADDR node.
//
// Arguments:
// tree - the GT_INDEX_ADDR node
//
void CodeGen::genCodeForIndexAddr(GenTreeIndexAddr* node)
Copy link
Copy Markdown

@CarolEidt CarolEidt Aug 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still feel that there is a lot of common code between this and the xarch implementation that could be factored out and hoisted to codegencommon.cpp to avoid duplication.

Copy link
Copy Markdown
Author

@pgavlin pgavlin Aug 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree: while the flow of the logic is similar, it differs substantially in whether/how the index is widened and what sequence of instructions is used to generate the address calculation. Refactoring this apart would produce something like

{
    GenTree* const base  = node->Arr();
    GenTree* const index = node->Index();

    genConsumeReg(base);
    genConsumeReg(index);

    // Generate the bounds check if necessary.
    if ((node->gtFlags & GTF_INX_RNGCHK) != 0)
    {
        generateRangeCheckForIndexAddr();
    }

    generateAddressForIndexAddr();

    genProduceReg(node);
}

which I don't think is a substantial improvement.

Copy link
Copy Markdown

@CarolEidt CarolEidt Aug 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, makes sense.

{
GenTree* const base = node->Arr();
GenTree* const index = node->Index();

genConsumeReg(base);
genConsumeReg(index);

const regNumber tmpReg = node->GetSingleTempReg();

// 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 and load the length into a register.
GenTreeAddrMode arrLenAddr(base->TypeGet(), base, nullptr, 0, static_cast<unsigned>(node->gtLenOffset));
arrLenAddr.gtRegNum = REG_NA;
arrLenAddr.SetContained();
arrLenAddr.gtNext = (GenTree*)(-1);

GenTreeIndir arrLen = indirForm(TYP_INT, &arrLenAddr);
arrLen.gtRegNum = tmpReg;
arrLen.ClearContained();

getEmitter()->emitInsLoadStoreOp(ins_Load(TYP_INT), emitTypeSize(TYP_INT), arrLen.gtRegNum, &arrLen);

#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.
if (index->TypeGet() == TYP_I_IMPL)
{
// Extend the array length as needed.
getEmitter()->emitIns_R_R(ins_Move_Extend(TYP_INT, true), EA_8BYTE, arrLen.gtRegNum, arrLen.gtRegNum);
}
#endif

// Generate the range check.
getEmitter()->emitInsBinary(INS_cmp, emitTypeSize(TYP_I_IMPL), index, &arrLen);
genJumpToThrowHlpBlk(genJumpKindForOper(GT_GE, CK_UNSIGNED), SCK_RNGCHK_FAIL, node->gtIndRngFailBB);
}

// Compute the address of the array element.
switch (node->gtElemSize)
{
case 1:
// dest = base + index
getEmitter()->emitIns_R_R_R(INS_add, emitTypeSize(node), node->gtRegNum, base->gtRegNum, index->gtRegNum);
break;

case 2:
case 4:
case 8:
case 16:
{
DWORD lsl;
BitScanForward(&lsl, node->gtElemSize);

// dest = base + index * scale
genScaledAdd(emitTypeSize(node), node->gtRegNum, base->gtRegNum, index->gtRegNum, lsl);
break;
}

default:
{
// tmp = scale
CodeGen::genSetRegToIcon(tmpReg, (ssize_t)node->gtElemSize, TYP_INT);

// dest = base + index * tmp
getEmitter()->emitIns_R_R_R_R(INS_MULADD, emitTypeSize(node), node->gtRegNum, node->gtRegNum,
index->gtRegNum, tmpReg);
break;
}
}

// dest = dest + elemOffs
getEmitter()->emitIns_R_R_I(INS_add, emitTypeSize(node), node->gtRegNum, node->gtRegNum, node->gtElemOffset);

genProduceReg(node);
}

//------------------------------------------------------------------------
// genCodeForIndir: Produce code for a GT_IND node.
//
Expand Down
1 change: 1 addition & 0 deletions src/jit/codegenlinear.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ void genCodeForShiftRMW(GenTreeStoreInd* storeInd);

void genCodeForCast(GenTreeOp* tree);
void genCodeForLclAddr(GenTree* tree);
void genCodeForIndexAddr(GenTreeIndexAddr* tree);
void genCodeForIndir(GenTreeIndir* tree);
void genCodeForNegNot(GenTree* tree);
void genCodeForLclVar(GenTreeLclVar* tree);
Expand Down
85 changes: 85 additions & 0 deletions src/jit/codegenxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1787,6 +1787,10 @@ void CodeGen::genCodeForTreeNode(GenTreePtr treeNode)
genLeaInstruction(treeNode->AsAddrMode());
break;

case GT_INDEX_ADDR:
genCodeForIndexAddr(treeNode->AsIndexAddr());
break;

case GT_IND:
genCodeForIndir(treeNode->AsIndir());
break;
Expand Down Expand Up @@ -4494,6 +4498,87 @@ void CodeGen::genCodeForStoreLclVar(GenTreeLclVar* tree)
}
}

//------------------------------------------------------------------------
// genCodeForIndexAddr: Produce code for a GT_INDEX_ADDR node.
//
// Arguments:
// tree - the GT_INDEX_ADDR node
//
void CodeGen::genCodeForIndexAddr(GenTreeIndexAddr* node)
{
GenTree* const base = node->Arr();
GenTree* const index = node->Index();

genConsumeReg(base);
genConsumeReg(index);

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();
arrLenAddr.gtNext = (GenTree*)(-1);

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.
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);
}
else
#endif
{
assert(varTypeIsIntegral(index->TypeGet()));

arrLen.gtRegNum = REG_NA;
arrLen.SetContained();
arrLen.gtNext = (GenTree*)(-1);
}

// Generate the range check.
getEmitter()->emitInsBinary(INS_cmp, emitTypeSize(TYP_I_IMPL), index, &arrLen);
genJumpToThrowHlpBlk(EJ_jae, SCK_RNGCHK_FAIL, node->gtIndRngFailBB);
}

// Compute the address of the array element.
switch (node->gtElemSize)
{
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));
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));
break;
}
}

genProduceReg(node);
}

//------------------------------------------------------------------------
// genCodeForIndir: Produce code for a GT_IND node.
//
Expand Down
22 changes: 22 additions & 0 deletions src/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4668,6 +4668,8 @@ class Compiler

void fgSetRngChkTarget(GenTreePtr tree, bool delay = true);

BasicBlock* fgSetRngChkTargetInner(SpecialCodeKind kind, bool delay, unsigned* stkDepth);

#if REARRANGE_ADDS
void fgMoveOpsLeft(GenTreePtr tree);
#endif
Expand Down Expand Up @@ -9320,6 +9322,26 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
return compRoot->m_arrayInfoMap;
}

inline bool TryGetArrayInfo(GenTreeIndir* indir, ArrayInfo* arrayInfo)
{
if ((indir->gtFlags & GTF_IND_ARR_INDEX) == 0)
{
return false;
}

if (indir->gtOp1->OperIs(GT_INDEX_ADDR))
{
GenTreeIndexAddr* const indexAddr = indir->gtOp1->AsIndexAddr();
*arrayInfo = ArrayInfo(indexAddr->gtElemType, indexAddr->gtElemSize, indexAddr->gtElemOffset,
indexAddr->gtStructElemClass);
return true;
}

bool found = GetArrayInfoMap()->Lookup(indir, arrayInfo);
assert(found);
return true;
}

NodeToUnsignedMap* m_memorySsaMap[MemoryKindCount];

// In some cases, we want to assign intermediate SSA #'s to memory states, and know what nodes create those memory
Expand Down
7 changes: 7 additions & 0 deletions src/jit/flowgraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18356,6 +18356,13 @@ void Compiler::fgSetTreeSeqHelper(GenTreePtr tree, bool isLIR)
noway_assert(!"DYN_BLK nodes should be sequenced as a special case");
break;

case GT_INDEX_ADDR:
// Evaluate the index first, then the array address
assert((tree->gtFlags & GTF_REVERSE_OPS) != 0);
fgSetTreeSeqHelper(tree->AsIndexAddr()->Index(), isLIR);
fgSetTreeSeqHelper(tree->AsIndexAddr()->Arr(), isLIR);
break;

default:
#ifdef DEBUG
gtDispTree(tree);
Expand Down
54 changes: 50 additions & 4 deletions src/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,7 @@ void GenTree::InitNodeSize()
GenTree::s_gtNodeSizes[GT_FTN_ADDR] = TREE_NODE_SZ_LARGE;
GenTree::s_gtNodeSizes[GT_BOX] = TREE_NODE_SZ_LARGE;
GenTree::s_gtNodeSizes[GT_INDEX] = TREE_NODE_SZ_LARGE;
GenTree::s_gtNodeSizes[GT_INDEX_ADDR] = TREE_NODE_SZ_LARGE;
GenTree::s_gtNodeSizes[GT_ARR_BOUNDS_CHECK] = TREE_NODE_SZ_LARGE;
#ifdef FEATURE_SIMD
GenTree::s_gtNodeSizes[GT_SIMD_CHK] = TREE_NODE_SZ_LARGE;
Expand Down Expand Up @@ -1301,6 +1302,12 @@ bool GenTree::Compare(GenTreePtr op1, GenTreePtr op2, bool swapOK)
return false;
}
break;
case GT_INDEX_ADDR:
if (op1->AsIndexAddr()->gtElemSize != op2->AsIndexAddr()->gtElemSize)
{
return false;
}
break;

// For the ones below no extra argument matters for comparison.
case GT_QMARK:
Expand Down Expand Up @@ -1876,6 +1883,9 @@ unsigned Compiler::gtHashValue(GenTree* tree)
case GT_INDEX:
hash += tree->gtIndex.gtIndElemSize;
break;
case GT_INDEX_ADDR:
hash += tree->AsIndexAddr()->gtElemSize;
break;
case GT_ALLOCOBJ:
hash = genTreeHashAdd(hash, static_cast<unsigned>(
reinterpret_cast<uintptr_t>(tree->gtAllocObj.gtAllocObjClsHnd)));
Expand Down Expand Up @@ -1938,6 +1948,7 @@ unsigned Compiler::gtHashValue(GenTree* tree)
case GT_ARR_INDEX:
case GT_QMARK:
case GT_INDEX:
case GT_INDEX_ADDR:
break;

#ifdef FEATURE_SIMD
Expand Down Expand Up @@ -4776,6 +4787,23 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree)
}
break;

case GT_INDEX_ADDR:
costEx = 6; // cmp reg,reg; jae throw; mov reg, [addrmode] (not taken)
costSz = 9; // jump to cold section

level = gtSetEvalOrder(tree->AsIndexAddr()->Index());
costEx += tree->AsIndexAddr()->Index()->gtCostEx;
costSz += tree->AsIndexAddr()->Index()->gtCostSz;

lvl2 = gtSetEvalOrder(tree->AsIndexAddr()->Arr());
if (level < lvl2)
{
level = lvl2;
}
costEx += tree->AsIndexAddr()->Arr()->gtCostEx;
costSz += tree->AsIndexAddr()->Arr()->gtCostSz;
break;

default:
#ifdef DEBUG
if (verbose)
Expand Down Expand Up @@ -5808,6 +5836,7 @@ bool GenTree::OperMayThrow()
#ifdef FEATURE_SIMD
case GT_SIMD_CHK:
#endif // FEATURE_SIMD
case GT_INDEX_ADDR:
return true;
default:
break;
Expand Down Expand Up @@ -7416,6 +7445,19 @@ GenTreePtr Compiler::gtCloneExpr(
}
break;

case GT_INDEX_ADDR:
{
GenTreeIndexAddr* asIndAddr = tree->AsIndexAddr();

copy = new (this, GT_INDEX_ADDR)
GenTreeIndexAddr(asIndAddr->Arr(), asIndAddr->Index(), asIndAddr->gtElemType,
asIndAddr->gtStructElemClass, asIndAddr->gtElemSize, asIndAddr->gtLenOffset,
asIndAddr->gtElemOffset);
copy->AsIndexAddr()->gtIndRngFailBB = asIndAddr->gtIndRngFailBB;
copy->AsIndexAddr()->gtStkDepth = asIndAddr->gtStkDepth;
}
break;

case GT_ALLOCOBJ:
{
GenTreeAllocObj* asAllocObj = tree->AsAllocObj();
Expand Down Expand Up @@ -9584,6 +9626,7 @@ void Compiler::gtDispNode(GenTreePtr tree, IndentStack* indentStack, __in __in_z
__fallthrough;

case GT_INDEX:
case GT_INDEX_ADDR:

if ((tree->gtFlags & (GTF_IND_VOLATILE | GTF_IND_UNALIGNED)) == 0) // We prefer printing V or U over R
{
Expand Down Expand Up @@ -15682,6 +15725,9 @@ CORINFO_CLASS_HANDLE Compiler::gtGetStructHandleIfPresent(GenTree* tree)
case GT_INDEX:
structHnd = tree->gtIndex.gtStructElemClass;
break;
case GT_INDEX_ADDR:
structHnd = tree->AsIndexAddr()->gtStructElemClass;
break;
case GT_FIELD:
info.compCompHnd->getFieldType(tree->gtField.gtFldHnd, &structHnd);
break;
Expand Down Expand Up @@ -15710,12 +15756,12 @@ CORINFO_CLASS_HANDLE Compiler::gtGetStructHandleIfPresent(GenTree* tree)
}
else
#endif
if (tree->gtFlags & GTF_IND_ARR_INDEX)
{
ArrayInfo arrInfo;
bool b = GetArrayInfoMap()->Lookup(tree, &arrInfo);
assert(b);
structHnd = EncodeElemType(arrInfo.m_elemType, arrInfo.m_elemStructType);
if (TryGetArrayInfo(tree->AsIndir(), &arrInfo))
{
structHnd = EncodeElemType(arrInfo.m_elemType, arrInfo.m_elemStructType);
}
}
break;
#ifdef FEATURE_SIMD
Expand Down
Loading