Skip to content

Commit

Permalink
Address TODO-FIELDs and delete quirks (#85735)
Browse files Browse the repository at this point in the history 
* Adjust a comment

* Simplify CreateAddressNodeForSimdHWIntrinsicCreate

No diffs.

* Limit the usages of gtNewFieldIndirNode

One tiny regression in a Regex method due to us not clearing GLOB_REF where not needed.

* Delete the side effects quirk

About neutral in improvements and regressions.

Both come from the differences in the handling of unused indirs and NULLCHECK nodes.

* Add a convenience gtNewFieldAddrNode overload
  • Loading branch information
@SingleAccretion
SingleAccretion authored May 6, 2023
1 parent 754d9ec commit e61e022
Show file tree
Hide file tree
Showing 7 changed files with 51 additions and 103 deletions.
5 changes: 5 additions & 0 deletions src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2815,6 +2815,11 @@ class Compiler
GenTree* obj = nullptr,
DWORD offset = 0);

GenTreeFieldAddr* gtNewFieldAddrNode(CORINFO_FIELD_HANDLE fldHnd, GenTree* obj, unsigned offset)
{
return gtNewFieldAddrNode(varTypeIsGC(obj) ? TYP_BYREF : TYP_I_IMPL, fldHnd, obj, offset);
}

GenTreeIndir* gtNewFieldIndirNode(var_types type, ClassLayout* layout, GenTreeFieldAddr* addr);

GenTreeIndexAddr* gtNewIndexAddr(GenTree* arrayOp,
Expand Down
11 changes: 0 additions & 11 deletions src/coreclr/jit/gentree.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16308,17 +16308,6 @@ bool Compiler::gtNodeHasSideEffects(GenTree* tree, GenTreeFlags flags)
{
return true;
}

// TODO-FIELD: delete this zero-diff quirk.
if (tree->OperIsIndir() && tree->AsIndir()->Addr()->OperIs(GT_FIELD_ADDR))
{
GenTreeFieldAddr* addr = tree->AsIndir()->Addr()->AsFieldAddr();
if (addr->IsInstance() && ((addr->gtFlags & GTF_FLD_DEREFERENCED) != 0) &&
fgAddrCouldBeNull(addr->GetFldObj()))
{
return true;
}
}
}

// Expressions declared as CSE by (e.g.) hoisting code are considered to have relevant side
Expand Down
15 changes: 3 additions & 12 deletions src/coreclr/jit/importer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1115,14 +1115,7 @@ GenTree* Compiler::impGetStructAddr(GenTree* structVal, unsigned curLevel, bool
case GT_IND:
if (willDeref)
{
GenTree* addr = structVal->AsIndir()->Addr();
if (addr->OperIs(GT_FIELD_ADDR))
{
// TODO-FIELD: delete this zero-diff quirk.
addr->gtFlags &= ~GTF_FLD_DEREFERENCED;
}

return addr;
return structVal->AsIndir()->Addr();
}
break;

Expand Down Expand Up @@ -9157,8 +9150,7 @@ void Compiler::impImportBlockCode(BasicBlock* block)
obj = impGetStructAddr(obj, CHECK_SPILL_ALL, true);
}

op1 = gtNewFieldAddrNode(varTypeIsGC(obj) ? TYP_BYREF : TYP_I_IMPL, resolvedToken.hField, obj,
fieldInfo.offset);
op1 = gtNewFieldAddrNode(resolvedToken.hField, obj, fieldInfo.offset);

#ifdef FEATURE_READYTORUN
if (fieldInfo.fieldAccessor == CORINFO_FIELD_INSTANCE_WITH_BASE)
Expand Down Expand Up @@ -9464,8 +9456,7 @@ void Compiler::impImportBlockCode(BasicBlock* block)
case CORINFO_FIELD_INSTANCE_WITH_BASE:
#endif
{
op1 = gtNewFieldAddrNode(varTypeIsGC(obj) ? TYP_BYREF : TYP_I_IMPL, resolvedToken.hField, obj,
fieldInfo.offset);
op1 = gtNewFieldAddrNode(resolvedToken.hField, obj, fieldInfo.offset);

#ifdef FEATURE_READYTORUN
if (fieldInfo.fieldAccessor == CORINFO_FIELD_INSTANCE_WITH_BASE)
Expand Down
21 changes: 9 additions & 12 deletions src/coreclr/jit/importercalls.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2897,10 +2897,9 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis,
CORINFO_FIELD_HANDLE lengthHnd = info.compCompHnd->getFieldInClass(clsHnd, 1);
const unsigned lengthOffset = info.compCompHnd->getFieldOffset(lengthHnd);

GenTreeFieldAddr* lengthFieldAddr =
gtNewFieldAddrNode(genActualType(ptrToSpan), lengthHnd, ptrToSpan, lengthOffset);
GenTreeFieldAddr* lengthFieldAddr = gtNewFieldAddrNode(lengthHnd, ptrToSpan, lengthOffset);
GenTree* length = gtNewIndir(TYP_INT, lengthFieldAddr);
lengthFieldAddr->SetIsSpanLength(true);
GenTree* length = gtNewFieldIndirNode(TYP_INT, nullptr, lengthFieldAddr);

GenTree* boundsCheck = new (this, GT_BOUNDS_CHECK) GenTreeBoundsChk(index, length, SCK_RNGCHK_FAIL);

Expand All @@ -2914,12 +2913,11 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis,
index = gtNewOperNode(GT_MUL, TYP_I_IMPL, index, sizeofNode);
}

CORINFO_FIELD_HANDLE ptrHnd = info.compCompHnd->getFieldInClass(clsHnd, 0);
const unsigned ptrOffset = info.compCompHnd->getFieldOffset(ptrHnd);
GenTreeFieldAddr* dataFieldAddr =
gtNewFieldAddrNode(genActualType(ptrToSpanClone), ptrHnd, ptrToSpanClone, ptrOffset);
GenTree* data = gtNewFieldIndirNode(TYP_BYREF, nullptr, dataFieldAddr);
GenTree* result = gtNewOperNode(GT_ADD, TYP_BYREF, data, index);
CORINFO_FIELD_HANDLE ptrHnd = info.compCompHnd->getFieldInClass(clsHnd, 0);
const unsigned ptrOffset = info.compCompHnd->getFieldOffset(ptrHnd);
GenTreeFieldAddr* dataFieldAddr = gtNewFieldAddrNode(ptrHnd, ptrToSpanClone, ptrOffset);
GenTree* data = gtNewIndir(TYP_BYREF, dataFieldAddr);
GenTree* result = gtNewOperNode(GT_ADD, TYP_BYREF, data, index);

// Prepare result
var_types resultType = JITtype2varType(sig->retType);
Expand Down Expand Up @@ -2962,10 +2960,9 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis,
CORINFO_FIELD_HANDLE lengthHnd = info.compCompHnd->getFieldInClass(clsHnd, 1);
const unsigned lengthOffset = info.compCompHnd->getFieldOffset(lengthHnd);

GenTreeFieldAddr* lengthFieldAddr =
gtNewFieldAddrNode(genActualType(ptrToSpan), lengthHnd, ptrToSpan, lengthOffset);
GenTreeFieldAddr* lengthFieldAddr = gtNewFieldAddrNode(lengthHnd, ptrToSpan, lengthOffset);
GenTree* lengthField = gtNewIndir(TYP_INT, lengthFieldAddr);
lengthFieldAddr->SetIsSpanLength(true);
GenTree* lengthField = gtNewFieldIndirNode(TYP_INT, nullptr, lengthFieldAddr);

return lengthField;
}
Expand Down
8 changes: 4 additions & 4 deletions src/coreclr/jit/importervectorization.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -787,10 +787,10 @@ GenTree* Compiler::impSpanEqualsOrStartsWith(bool startsWith, CORINFO_SIG_INFO*
GenTreeLclVar* spanObjRefLcl = gtNewLclvNode(spanObjRef, TYP_BYREF);
GenTreeLclVar* spanDataTmpLcl = gtNewLclvNode(spanDataTmp, TYP_BYREF);

GenTreeFieldAddr* spanLengthAddr = gtNewFieldAddrNode(TYP_BYREF, lengthHnd, gtClone(spanObjRefLcl), lengthOffset);
GenTree* spanLength = gtNewFieldIndirNode(TYP_INT, nullptr, spanLengthAddr);
GenTreeFieldAddr* spanDataAddr = gtNewFieldAddrNode(TYP_BYREF, pointerHnd, spanObjRefLcl);
GenTree* spanData = gtNewFieldIndirNode(TYP_BYREF, nullptr, spanDataAddr);
GenTreeFieldAddr* spanLengthAddr = gtNewFieldAddrNode(lengthHnd, gtClone(spanObjRefLcl), lengthOffset);
GenTree* spanLength = gtNewIndir(TYP_INT, spanLengthAddr);
GenTreeFieldAddr* spanDataAddr = gtNewFieldAddrNode(pointerHnd, spanObjRefLcl, 0);
GenTree* spanData = gtNewIndir(TYP_BYREF, spanDataAddr);

GenTree* unrolled =
impExpandHalfConstEquals(spanDataTmpLcl, spanLength, false, startsWith, (WCHAR*)str, cnsLength, 0, cmpMode);
Expand Down
16 changes: 4 additions & 12 deletions src/coreclr/jit/morph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4436,20 +4436,12 @@ GenTree* Compiler::fgMorphIndexAddr(GenTreeIndexAddr* indexAddr)
// If the arrRef or index expressions involves an assignment, a call, or reads from global memory,
// then we *must* allocate a temporary in which to "localize" those values, to ensure that the
// same values are used in the bounds check and the actual dereference.
// Also we allocate the temporary when the expression is sufficiently complex/expensive.
// Also we allocate the temporary when the expression is sufficiently complex/expensive. We special
// case some simple nodes for which CQ analysis shows it is a little better to do that here than
// leaving them to CSE.
//
// TODO-FIELD: review below comment.
// TODO-Bug: GLOB_REF is not yet set for all trees in pre-order morph.
//
// Note that if the expression is a GT_FIELD, it has not yet been morphed so its true complexity is
// not exposed. Without that condition there are cases of local struct fields that were previously,
// needlessly, marked as GTF_GLOB_REF, and when that was fixed, there were some regressions that
// were mostly ameliorated by adding this condition.
//
// Likewise, allocate a temporary if the expression is a GT_LCL_FLD node. These used to be created
// after fgMorphIndexAddr from GT_FIELD trees so this preserves the existing behavior. This is
// perhaps a decision that should be left to CSE but FX diffs show that it is slightly better to
// do this here. Likewise for implicit byrefs.

if (((arrRef->gtFlags & (GTF_ASG | GTF_CALL | GTF_GLOB_REF)) != 0) ||
gtComplexityExceeds(arrRef, MAX_ARR_COMPLEXITY) || arrRef->OperIs(GT_LCL_FLD) ||
(arrRef->OperIs(GT_LCL_VAR) && lvaIsLocalImplicitlyAccessedByRef(arrRef->AsLclVar()->GetLclNum())))
Expand Down
78 changes: 26 additions & 52 deletions src/coreclr/jit/simd.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -689,69 +689,43 @@ bool Compiler::areArgumentsContiguous(GenTree* op1, GenTree* op2)
GenTree* Compiler::CreateAddressNodeForSimdHWIntrinsicCreate(GenTree* tree, var_types simdBaseType, unsigned simdSize)
{
assert(tree->OperIs(GT_IND));
GenTree* addr = tree->AsIndir()->Addr();
GenTree* byrefNode = nullptr;
unsigned offset = 0;
var_types baseType = tree->gtType;
GenTree* addr = tree->AsIndir()->Addr();

if (addr->OperIs(GT_FIELD_ADDR))
{
assert(addr->AsFieldAddr()->IsInstance());

// If the field is directly from a struct, then in this case, we should set this
// struct's lvUsedInSIMDIntrinsic as true, so that this sturct won't be promoted.
GenTree* objRef = addr->AsFieldAddr()->GetFldObj();
if (objRef->IsLclVarAddr())
if (objRef->IsLclVarAddr() && varTypeIsSIMD(lvaGetDesc(objRef->AsLclFld())))
{
// If the field is directly from a struct, then in this case,
// we should set this struct's lvUsedInSIMDIntrinsic as true,
// so that this sturct won't be promoted.
// e.g. s.x x is a field, and s is a struct, then we should set the s's lvUsedInSIMDIntrinsic as true.
// so that s won't be promoted.
// Notice that if we have a case like s1.s2.x. s1 s2 are struct, and x is a field, then it is possible that
// s1 can be promoted, so that s2 can be promoted. The reason for that is if we don't allow s1 to be
// promoted, then this will affect the other optimizations which are depend on s1's struct promotion.
// TODO-CQ:
// In future, we should optimize this case so that if there is a nested field like s1.s2.x and s1.s2.x's
// address is used for initializing the vector, then s1 can be promoted but s2 can't.
if (varTypeIsSIMD(lvaGetDesc(objRef->AsLclFld())))
{
setLclRelatedToSIMDIntrinsic(objRef);
}
setLclRelatedToSIMDIntrinsic(objRef);
}

// TODO-FIELD: this seems unnecessary. Simply "return addr;"?
byrefNode = gtCloneExpr(objRef);
assert(byrefNode != nullptr);
offset = addr->AsFieldAddr()->gtFldOffset;
}
else
{
GenTree* arrayRef = addr->AsIndexAddr()->Arr();
GenTree* index = addr->AsIndexAddr()->Index();
assert(index->IsCnsIntOrI());

GenTree* checkIndexExpr = nullptr;
unsigned indexVal = (unsigned)index->AsIntCon()->gtIconVal;
offset = indexVal * genTypeSize(tree->TypeGet());

// Generate the boundary check exception.
// The length for boundary check should be the maximum index number which should be
// (first argument's index number) + (how many array arguments we have) - 1
// = indexVal + arrayElementsCount - 1
unsigned arrayElementsCount = simdSize / genTypeSize(simdBaseType);
checkIndexExpr = gtNewIconNode(indexVal + arrayElementsCount - 1);
GenTreeArrLen* arrLen = gtNewArrLen(TYP_INT, arrayRef, (int)OFFSETOF__CORINFO_Array__length, compCurBB);
GenTreeBoundsChk* arrBndsChk =
new (this, GT_BOUNDS_CHECK) GenTreeBoundsChk(checkIndexExpr, arrLen, SCK_ARG_RNG_EXCPN);

offset += OFFSETOF__CORINFO_Array__data;
byrefNode = gtNewOperNode(GT_COMMA, arrayRef->TypeGet(), arrBndsChk, gtCloneExpr(arrayRef));
return addr;
}

GenTree* address = byrefNode;
if (offset != 0)
{
address = gtNewOperNode(GT_ADD, TYP_BYREF, address, gtNewIconNode(offset, TYP_I_IMPL));
}
GenTree* arrayRef = addr->AsIndexAddr()->Arr();
GenTree* index = addr->AsIndexAddr()->Index();
assert(index->IsCnsIntOrI());

unsigned indexVal = (unsigned)index->AsIntCon()->gtIconVal;
unsigned offset = indexVal * genTypeSize(tree->TypeGet());

// Generate the boundary check exception.
// The length for boundary check should be the maximum index number which should be
// (first argument's index number) + (how many array arguments we have) - 1 = indexVal + arrayElementsCount - 1
//
unsigned arrayElementsCount = simdSize / genTypeSize(simdBaseType);
GenTree* checkIndexExpr = gtNewIconNode(indexVal + arrayElementsCount - 1);
GenTreeArrLen* arrLen = gtNewArrLen(TYP_INT, arrayRef, (int)OFFSETOF__CORINFO_Array__length, compCurBB);
GenTreeBoundsChk* arrBndsChk =
new (this, GT_BOUNDS_CHECK) GenTreeBoundsChk(checkIndexExpr, arrLen, SCK_ARG_RNG_EXCPN);

offset += OFFSETOF__CORINFO_Array__data;
GenTree* address = gtNewOperNode(GT_COMMA, arrayRef->TypeGet(), arrBndsChk, gtCloneExpr(arrayRef));
address = gtNewOperNode(GT_ADD, TYP_BYREF, address, gtNewIconNode(offset, TYP_I_IMPL));

return address;
}
Expand Down

0 comments on commit e61e022

Please sign in to comment.