Skip to content

Commit

Permalink
Simplify impAssignStruct[Ptr] and gtNewBlkOpNode (#78252)
Browse files Browse the repository at this point in the history 
* Simple start

Avoid creating COMMAs on the LHS.
Delete unnecessary code.

* Implement "impAssignStructPtr" via "impAssignStruct"

Essentially, swap them.

* Delete a bunch of dead code

* Delete the struct handle parameter

Not needed.

* Fix RET_EXPR type mismatch

* Delete "gtBlockOpInit"

Inline it into "gtNewBlkOpNode", its only user.

* Don't use gtNewBlkOpNode for primitives

This was creating "ASG(long, int)" and similar invalid IR.

* Simplify "gtNewBlkOpNode" interface

Delete "isCopyBlock" now that it can be inferred.
Default "isVolatile" to "false".

* Fix formatting
  • Loading branch information
@SingleAccretion
SingleAccretion authored Dec 1, 2022
1 parent ea060e6 commit 91f114b
Show file tree
Hide file tree
Showing 11 changed files with 171 additions and 448 deletions.
22 changes: 7 additions & 15 deletions src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2357,15 +2357,12 @@ class Compiler

GenTreeLclVar* gtNewStoreLclVar(unsigned dstLclNum, GenTree* src);

GenTree* gtNewBlkOpNode(GenTree* dst, GenTree* srcOrFillVal, bool isVolatile, bool isCopyBlock);
GenTree* gtNewBlkOpNode(GenTree* dst, GenTree* srcOrFillVal, bool isVolatile = false);

GenTree* gtNewPutArgReg(var_types type, GenTree* arg, regNumber argReg);

GenTree* gtNewBitCastNode(var_types type, GenTree* arg);

protected:
void gtBlockOpInit(GenTree* result, GenTree* dst, GenTree* srcOrFillVal, bool isVolatile);

public:
GenTreeObj* gtNewObjNode(ClassLayout* layout, GenTree* addr);
GenTreeObj* gtNewObjNode(CORINFO_CLASS_HANDLE structHnd, GenTree* addr);
Expand Down Expand Up @@ -3882,13 +3879,12 @@ class Compiler
CORINFO_CLASS_HANDLE structHnd,
unsigned curLevel,
Statement** pAfterStmt DEBUGARG(const char* reason));
GenTree* impAssignStruct(GenTree* dest,
GenTree* src,
CORINFO_CLASS_HANDLE structHnd,
unsigned curLevel,
Statement** pAfterStmt = nullptr,
const DebugInfo& di = DebugInfo(),
BasicBlock* block = nullptr);
GenTree* impAssignStruct(GenTree* dest,
GenTree* src,
unsigned curLevel,
Statement** pAfterStmt = nullptr,
const DebugInfo& di = DebugInfo(),
BasicBlock* block = nullptr);
GenTree* impAssignStructPtr(GenTree* dest,
GenTree* src,
CORINFO_CLASS_HANDLE structHnd,
Expand Down Expand Up @@ -4157,10 +4153,6 @@ class Compiler
void impLoadLoc(unsigned ilLclNum, IL_OFFSET offset);
bool impReturnInstruction(int prefixFlags, OPCODE& opcode);

#ifdef TARGET_ARM
void impMarkLclDstNotPromotable(unsigned tmpNum, GenTree* op, CORINFO_CLASS_HANDLE hClass);
#endif

// A free list of linked list nodes used to represent to-do stacks of basic blocks.
struct BlockListNode
{
Expand Down
7 changes: 2 additions & 5 deletions src/coreclr/jit/fginline.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitor<Substi
}

GenTree* dst = m_compiler->gtNewLclvNode(lclNum, varDsc->TypeGet());
GenTree* asg = m_compiler->gtNewBlkOpNode(dst, src, /* isVolatile */ false, /* isCopyBlock */ true);
GenTree* asg = m_compiler->gtNewBlkOpNode(dst, src);

// If inlinee was comma, new inlinee is (, , , lcl = inlinee).
if (inlinee->OperIs(GT_COMMA))
Expand Down Expand Up @@ -1743,10 +1743,7 @@ Statement* Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo)
}
else
{
tree = gtNewBlkOpNode(gtNewLclvNode(tmpNum, lclTyp), // Dest
gtNewIconNode(0), // Value
false, // isVolatile
false); // not copyBlock
tree = gtNewBlkOpNode(gtNewLclvNode(tmpNum, lclTyp), gtNewIconNode(0));

newStmt = gtNewStmt(tree, callDI);
fgInsertStmtAfter(block, afterStmt, newStmt);
Expand Down
126 changes: 40 additions & 86 deletions src/coreclr/jit/gentree.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -626,6 +626,10 @@ ClassLayout* GenTree::GetLayout(Compiler* compiler) const
structHnd = AsCall()->gtRetClsHnd;
break;

case GT_RET_EXPR:
structHnd = AsRetExpr()->gtInlineCandidate->gtRetClsHnd;
break;

default:
unreached();
}
Expand Down Expand Up @@ -7184,7 +7188,7 @@ GenTree* Compiler::gtNewZeroConNode(var_types type)
{
GenTree* zero;

switch (type)
switch (genActualType(type))
{
case TYP_INT:
case TYP_REF:
Expand Down Expand Up @@ -7819,7 +7823,7 @@ GenTree* Compiler::gtNewBlockVal(GenTree* addr, unsigned size)
// extended to the size of the assignment when an initBlk is transformed
// to an assignment of a primitive type.
// This performs the appropriate extension.

//
void GenTreeIntCon::FixupInitBlkValue(var_types asgType)
{
assert(varTypeIsIntegralOrI(asgType));
Expand Down Expand Up @@ -8017,32 +8021,33 @@ void GenTreeOp::CheckDivideByConstOptimized(Compiler* comp)
}
}

//
//------------------------------------------------------------------------
// gtBlockOpInit: Initializes a BlkOp GenTree
// gtNewBlkOpNode: Creates a GenTree for a block (struct) assignment.
//
// Arguments:
// result - an assignment node that is to be initialized.
// dst - the target (destination) we want to either initialize or copy to.
// src - the init value for InitBlk or the source struct for CpBlk/CpObj.
// isVolatile - specifies whether this node is a volatile memory operation.
// dst - The destination node: local var / block node.
// srcOrFillVall - The value to assign for CopyBlk, the integer "fill" for InitBlk
// isVolatile - Whether this is a volatile memory operation or not.
//
// Assumptions:
// 'result' is an assignment that is newly constructed.
// If 'dst' is TYP_STRUCT, then it must be a block node or lclVar.
// Return Value:
// Returns the newly constructed and initialized block operation.
//
// Notes:
// This procedure centralizes all the logic to both enforce proper structure and
// to properly construct any InitBlk/CpBlk node.

void Compiler::gtBlockOpInit(GenTree* result, GenTree* dst, GenTree* srcOrFillVal, bool isVolatile)
GenTree* Compiler::gtNewBlkOpNode(GenTree* dst, GenTree* srcOrFillVal, bool isVolatile)
{
if (!result->OperIsBlkOp())
assert(varTypeIsStruct(dst) && (dst->OperIsBlk() || dst->OperIsLocal() || dst->OperIs(GT_FIELD)));

bool isCopyBlock = srcOrFillVal->TypeGet() == dst->TypeGet();
if (!isCopyBlock) // InitBlk
{
assert(dst->TypeGet() != TYP_STRUCT);
return;
assert(genActualTypeIsInt(srcOrFillVal));
if (!srcOrFillVal->IsIntegralConst(0))
{
srcOrFillVal = gtNewOperNode(GT_INIT_VAL, TYP_INT, srcOrFillVal);
}
}

GenTree* result = gtNewAssignNode(dst, srcOrFillVal);

/* In the case of CpBlk, we want to avoid generating
* nodes where the source and destination are the same
* because of two reasons, first, is useless, second
Expand All @@ -8061,7 +8066,7 @@ void Compiler::gtBlockOpInit(GenTree* result, GenTree* dst, GenTree* srcOrFillVa
* surface if struct promotion is ON (which is the case on x86/arm). But still the
* fundamental issue exists that needs to be addressed.
*/
if (result->OperIsCopyBlkOp())
if (isCopyBlock)
{
GenTree* currSrc = srcOrFillVal;
GenTree* currDst = dst;
Expand All @@ -8078,88 +8083,37 @@ void Compiler::gtBlockOpInit(GenTree* result, GenTree* dst, GenTree* srcOrFillVa
if (currSrc->OperGet() == GT_LCL_VAR && currDst->OperGet() == GT_LCL_VAR &&
currSrc->AsLclVarCommon()->GetLclNum() == currDst->AsLclVarCommon()->GetLclNum())
{
// Make this a NOP
// TODO-Cleanup: probably doesn't matter, but could do this earlier and avoid creating a GT_ASG
result->gtBashToNOP();
return;
result->gtBashToNOP(); // Make this a NOP.
return result;
}
}

// Propagate all effect flags from children
result->gtFlags |= dst->gtFlags & GTF_ALL_EFFECT;
result->gtFlags |= result->AsOp()->gtOp2->gtFlags & GTF_ALL_EFFECT;

result->gtFlags |= (dst->gtFlags & GTF_EXCEPT) | (srcOrFillVal->gtFlags & GTF_EXCEPT);

if (isVolatile)
{
result->gtFlags |= GTF_BLK_VOLATILE;
}

#ifdef FEATURE_SIMD
if (result->OperIsCopyBlkOp() && varTypeIsSIMD(srcOrFillVal))
// If the source is a SIMD/HWI node of SIMD type, then the dst lclvar struct
// should be labeled as simd intrinsic related struct. This is done so that
// we do not promote the local, thus avoiding conflicting access methods
// (fields vs. whole-register).
if (varTypeIsSIMD(srcOrFillVal) && srcOrFillVal->OperIsSimdOrHWintrinsic())
{
// If the source is a GT_SIMD node of SIMD type, then the dst lclvar struct
// should be labeled as simd intrinsic related struct.
// This is done so that the morpher can transform any field accesses into
// intrinsics, thus avoiding conflicting access methods (fields vs. whole-register).

GenTree* src = srcOrFillVal;
if (src->OperIsIndir() && (src->AsIndir()->Addr()->OperGet() == GT_ADDR))
// TODO-Cleanup: similar logic already exists in "gtNewAssignNode",
// however, it is not enabled for x86. Fix that and delete this code.
if (dst->OperIsBlk() && (dst->AsIndir()->Addr()->OperGet() == GT_ADDR))
{
src = src->AsIndir()->Addr()->gtGetOp1();
dst = dst->AsIndir()->Addr()->gtGetOp1();
}
#ifdef FEATURE_HW_INTRINSICS
if ((src->OperGet() == GT_SIMD) || (src->OperGet() == GT_HWINTRINSIC))
#else
if (src->OperGet() == GT_SIMD)
#endif // FEATURE_HW_INTRINSICS
{
if (dst->OperIsBlk() && (dst->AsIndir()->Addr()->OperGet() == GT_ADDR))
{
dst = dst->AsIndir()->Addr()->gtGetOp1();
}

if (dst->OperIsLocal() && varTypeIsStruct(dst))
{
setLclRelatedToSIMDIntrinsic(dst);
}
}
}
#endif // FEATURE_SIMD
}

//------------------------------------------------------------------------
// gtNewBlkOpNode: Creates a GenTree for a block (struct) assignment.
//
// Arguments:
// dst - The destination node: local var / block node.
// srcOrFillVall - The value to assign for CopyBlk, the integer "fill" for InitBlk
// isVolatile - Whether this is a volatile memory operation or not.
// isCopyBlock - True if this is a block copy (rather than a block init).
//
// Return Value:
// Returns the newly constructed and initialized block operation.
//
GenTree* Compiler::gtNewBlkOpNode(GenTree* dst, GenTree* srcOrFillVal, bool isVolatile, bool isCopyBlock)
{
assert(dst->OperIsBlk() || dst->OperIsLocal());

if (!isCopyBlock)
{
// InitBlk
assert(varTypeIsIntegral(srcOrFillVal));
if (varTypeIsStruct(dst))
if (dst->OperIsLocal() && varTypeIsStruct(dst))
{
if (!srcOrFillVal->IsIntegralConst(0))
{
srcOrFillVal = gtNewOperNode(GT_INIT_VAL, TYP_INT, srcOrFillVal);
}
setLclRelatedToSIMDIntrinsic(dst);
}
}
#endif // FEATURE_SIMD

GenTree* result = gtNewAssignNode(dst, srcOrFillVal);
gtBlockOpInit(result, dst, srcOrFillVal, isVolatile);
return result;
}

Expand Down Expand Up @@ -15785,7 +15739,7 @@ GenTree* Compiler::gtNewTempAssign(
}

valx->gtFlags |= GTF_DONT_CSE;
asg = impAssignStruct(dest, val, valStructHnd, CHECK_SPILL_NONE, pAfterStmt, di, block);
asg = impAssignStruct(dest, val, CHECK_SPILL_NONE, pAfterStmt, di, block);
}
else
{
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/jit/gschecks.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,7 @@ void Compiler::gsParamsToShadows()
{
assert(shadowVarDsc->GetLayout() != nullptr);
assert(shadowVarDsc->lvExactSize != 0);
opAssign = gtNewBlkOpNode(dst, src, false, true);
opAssign = gtNewBlkOpNode(dst, src);
}
else
{
Expand Down Expand Up @@ -618,7 +618,7 @@ void Compiler::gsParamsToShadows()
GenTree* opAssign = nullptr;
if (varDsc->TypeGet() == TYP_STRUCT)
{
opAssign = gtNewBlkOpNode(dst, src, false, true);
opAssign = gtNewBlkOpNode(dst, src);
}
else
{
Expand Down
Loading

0 comments on commit 91f114b

Please sign in to comment.