Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eliminate duplicate zero initializations more aggressively. #31960

Merged
merged 1 commit into from
Feb 11, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 9 additions & 0 deletions src/coreclr/src/jit/codegencommon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4656,6 +4656,9 @@ void CodeGen::genCheckUseBlockInit()
continue;
}

// TODO-Review: The code below is currently unreachable. We are guaranteed to execute one of the
// 'continue' statements above.
#if 0
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want to hold up getting this merged - looks great BTW - but why didn't you just delete this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to go back and see if any of this needs to be resurrected. It looks like part of the unreachable code was supposed to be responsible for correctness. I'll clean this up in a subsequent PR.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps open an issue for follow up on this, so we don't forget?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a comment to the issue that tracks improving the heuristic: #8890 (comment)

/* If we don't know lifetimes of variables, must be conservative */
if (!compiler->backendRequiresLocalVarLifetimes())
{
Expand Down Expand Up @@ -4690,6 +4693,7 @@ void CodeGen::genCheckUseBlockInit()
{
largeGcStructs++;
}
#endif
}

/* Don't forget about spill temps that hold pointers */
Expand Down Expand Up @@ -4718,6 +4722,11 @@ void CodeGen::genCheckUseBlockInit()
// Secondary factor is the presence of large structs that
// potentially only need some fields set to zero. We likely don't
// model this very well, but have left the logic as is for now.

// Compiler::fgVarNeedsExplicitZeroInit relies on this logic to
// find structs that are guaranteed to be block initialized.
// If this logic changes, Compiler::fgVarNeedsExplicitZeroInit needs
// to be modified.
CLANG_FORMAT_COMMENT_ANCHOR;

#ifdef TARGET_64BIT
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/src/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -4534,8 +4534,8 @@ class Compiler

unsigned fgSsaPassesCompleted; // Number of times fgSsaBuild has been run.

// Returns "true" if a struct temp of the given type requires needs zero init in this block
inline bool fgStructTempNeedsExplicitZeroInit(LclVarDsc* varDsc, BasicBlock* block);
// Returns "true" if the variable needs explicit zero initialization.
inline bool fgVarNeedsExplicitZeroInit(LclVarDsc* varDsc, bool bbInALoop, bool bbIsReturn);

// The value numbers for this compilation.
ValueNumStore* vnStore;
Expand Down
55 changes: 44 additions & 11 deletions src/coreclr/src/jit/compiler.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -4120,26 +4120,59 @@ inline void Compiler::CLR_API_Leave(API_ICorJitInfo_Names ename)
#endif // MEASURE_CLRAPI_CALLS

//------------------------------------------------------------------------------
// fgStructTempNeedsExplicitZeroInit : Check whether temp struct needs
// explicit zero initialization in this basic block.
// fgVarNeedsExplicitZeroInit : Check whether the variable needs an explicit zero initialization.
//
// Arguments:
// varDsc - struct local var description
// block - basic block to check
// varDsc - local var description
// bbInALoop - true if the basic block may be in a loop
// bbIsReturn - true if the basic block always returns
//
// Returns:
// true if the struct temp needs explicit zero-initialization in this basic block;
// true if the var needs explicit zero-initialization in this basic block;
// false otherwise
//
// Notes:
// If compInitMem is true, structs with GC pointer fields and long-lifetime structs
// are fully zero-initialized in the prologue. Therefore, we don't need to insert
// zero-initialization in this block if it is not in a loop.
// If the variable is not being initialized in a loop, we can avoid explicit zero initialization if
// - the variable is a gc pointer, or
// - the variable is a struct with gc pointer fields and either all fields are gc pointer fields
// or the struct is big enough to guarantee block initialization, or
// - compInitMem is set and the variable has a long lifetime or has gc fields.
// In these cases we will insert zero-initialization in the prolog if necessary.

bool Compiler::fgStructTempNeedsExplicitZeroInit(LclVarDsc* varDsc, BasicBlock* block)
bool Compiler::fgVarNeedsExplicitZeroInit(LclVarDsc* varDsc, bool bbInALoop, bool bbIsReturn)
{
return !info.compInitMem || ((block->bbFlags & BBF_BACKWARD_JUMP) != 0) ||
(!varDsc->HasGCPtr() && varDsc->lvIsTemp);
if (bbInALoop && !bbIsReturn)
{
return true;
}

if (varTypeIsGC(varDsc->lvType))
{
return false;
}

if ((varDsc->lvType == TYP_STRUCT) && varDsc->HasGCPtr())
{
ClassLayout* layout = varDsc->GetLayout();
if (layout->GetSlotCount() == layout->GetGCPtrCount())
{
return false;
}

// Below conditions guarantee block initialization, which will initialize
// all struct fields. If the logic for block initialization in CodeGen::genCheckUseBlockInit()
// changes, these conditions need to be updated.
#ifdef TARGET_64BIT
if (roundUp(varDsc->lvSize(), TARGET_POINTER_SIZE) / sizeof(int) > 8)
#else
if (roundUp(varDsc->lvSize(), TARGET_POINTER_SIZE) / sizeof(int) > 4)
#endif
{
return false;
}
}

return !info.compInitMem || (varDsc->lvIsTemp && !varDsc->HasGCPtr());
}

/*****************************************************************************/
Expand Down
44 changes: 28 additions & 16 deletions src/coreclr/src/jit/flowgraph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23579,10 +23579,15 @@ Statement* Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo)

CORINFO_METHOD_INFO* InlineeMethodInfo = InlineeCompiler->info.compMethodInfo;

unsigned lclCnt = InlineeMethodInfo->locals.numArgs;
unsigned lclCnt = InlineeMethodInfo->locals.numArgs;
bool bbInALoop = (block->bbFlags & BBF_BACKWARD_JUMP) != 0;
bool bbIsReturn = block->bbJumpKind == BBJ_RETURN;

// Does callee contain any zero-init local?
if ((lclCnt != 0) && (InlineeMethodInfo->options & CORINFO_OPT_INIT_LOCALS) != 0)
// If the callee contains zero-init locals, we need to explicitly initialize them if we are
// in a loop or if the caller doesn't have compInitMem set. Otherwise we can rely on the
// normal logic in the caller to insert zero-init in the prolog if necessary.
if ((lclCnt != 0) && ((InlineeMethodInfo->options & CORINFO_OPT_INIT_LOCALS) != 0) &&
((bbInALoop && !bbIsReturn) || !info.compInitMem))
{

#ifdef DEBUG
Expand All @@ -23596,9 +23601,20 @@ Statement* Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo)
{
unsigned tmpNum = inlineInfo->lclTmpNum[lclNum];

// Is the local used at all?
// If the local is used check whether we need to insert explicit zero initialization.
if (tmpNum != BAD_VAR_NUM)
{
if (!fgVarNeedsExplicitZeroInit(lvaGetDesc(tmpNum), bbInALoop, bbIsReturn))
{
#ifdef DEBUG
if (verbose)
{
printf("\nSkipping zero initialization of V%02u\n", tmpNum);
}
#endif // DEBUG
continue;
}

var_types lclTyp = (var_types)lvaTable[tmpNum].lvType;
noway_assert(lclTyp == lclVarInfo[lclNum + inlineInfo->argCnt].lclTypeInfo);

Expand All @@ -23613,18 +23629,14 @@ Statement* Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo)
{
CORINFO_CLASS_HANDLE structType =
lclVarInfo[lclNum + inlineInfo->argCnt].lclVerTypeInfo.GetClassHandle();

if (fgStructTempNeedsExplicitZeroInit(lvaTable + tmpNum, block))
{
tree = gtNewBlkOpNode(gtNewLclvNode(tmpNum, lclTyp), // Dest
gtNewIconNode(0), // Value
false, // isVolatile
false); // not copyBlock

newStmt = gtNewStmt(tree, callILOffset);
fgInsertStmtAfter(block, afterStmt, newStmt);
afterStmt = newStmt;
}
tree = gtNewBlkOpNode(gtNewLclvNode(tmpNum, lclTyp), // Dest
gtNewIconNode(0), // Value
false, // isVolatile
false); // not copyBlock

newStmt = gtNewStmt(tree, callILOffset);
fgInsertStmtAfter(block, afterStmt, newStmt);
afterStmt = newStmt;
Copy link
Member

Choose a reason for hiding this comment

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

Can you add jitdump here for the case where the local is used by we decide we don't need to zero init?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, done.

}

#ifdef DEBUG
Expand Down
8 changes: 7 additions & 1 deletion src/coreclr/src/jit/importer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13819,7 +13819,13 @@ void Compiler::impImportBlockCode(BasicBlock* block)
// and potentially exploitable.
lvaSetStruct(lclNum, resolvedToken.hClass, true /* unsafe value cls check */);
}
if (compIsForInlining() || fgStructTempNeedsExplicitZeroInit(lvaTable + lclNum, block))

bool bbInALoop =
(compIsForInlining() && ((impInlineInfo->iciBlock->bbFlags & BBF_BACKWARD_JUMP) != 0)) ||
((block->bbFlags & BBF_BACKWARD_JUMP) != 0);
bool bbIsReturn = (block->bbJumpKind == BBJ_RETURN) &&
(!compIsForInlining() || (impInlineInfo->iciBlock->bbJumpKind == BBJ_RETURN));
if (fgVarNeedsExplicitZeroInit(lvaGetDesc(lclNum), bbInALoop, bbIsReturn))
{
// Append a tree to zero-out the temp
newObjThisPtr = gtNewLclvNode(lclNum, lvaTable[lclNum].TypeGet());
Expand Down
6 changes: 4 additions & 2 deletions src/coreclr/src/jit/objectalloc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -510,8 +510,10 @@ unsigned int ObjectAllocator::MorphAllocObjNodeIntoStackAlloc(GenTreeAllocObj* a
const int unsafeValueClsCheck = true;
comp->lvaSetStruct(lclNum, allocObj->gtAllocObjClsHnd, unsafeValueClsCheck);

// Initialize the object memory if necessary
if (comp->fgStructTempNeedsExplicitZeroInit(comp->lvaTable + lclNum, block))
// Initialize the object memory if necessary.
bool bbInALoop = (block->bbFlags & BBF_BACKWARD_JUMP) != 0;
bool bbIsReturn = block->bbJumpKind == BBJ_RETURN;
if (comp->fgVarNeedsExplicitZeroInit(comp->lvaGetDesc(lclNum), bbInALoop, bbIsReturn))
{
//------------------------------------------------------------------------
// STMTx (IL 0x... ???)
Expand Down