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

JIT: improve types for single def locals and temps #10471

Merged
merged 3 commits into from
Mar 29, 2017
Merged

JIT: improve types for single def locals and temps #10471

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b744613
JIT: improve types for single def locals and temps
AndyAyersMS Mar 24, 2017
0f5141f
incorporate review feedback
AndyAyersMS Mar 27, 2017
45ae3dc
more review feedback
AndyAyersMS Mar 29, 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
12 changes: 8 additions & 4 deletions src/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,8 +252,9 @@ class LclVarDsc
unsigned char lvStackByref : 1; // This is a compiler temporary of TYP_BYREF that is known to point into our local
// stack frame.

unsigned char lvArgWrite : 1; // variable is a parameter and STARG was used on it
unsigned char lvIsTemp : 1; // Short-lifetime compiler temp
unsigned char lvArgWrite : 1; // there is at least one STLOC or STARG on this local
unsigned char lvMultipleArgWrite : 1; // there is more than one STLOC on this local
Copy link

Choose a reason for hiding this comment

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

IMO these names are confusing now, especially the fact that lvMultipleArgWrite doesn't apply to args/starg. Maybe something more like HasStore/HasMultipleStores?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's an asymmetry between args and locals that I wrestled with -- args are implicitly written once, so a single starg or ldarga creates a multiple-def arg, whereas for locals we need to see two of stloc or ldloca to have a multiple def local. I could resolve this by setting lvArgWrite when the arg temp is allocated and update the logic for args and then the fields would have same meaning. Does that seem preferable?

store seems a bit too specific for the name since these also apply to address-of operators. I'd use definition but then the overlap with lvISingleDef would be even more confusing. How about HasSingleWrite and HasMultipleWrites?

Copy link

Choose a reason for hiding this comment

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

I'd use definition but then the overlap with lvISingleDef would be even more confusing. How about HasSingleWrite and HasMultipleWrites?

I thought about definition and the same thing occurred to me. I picked "store" because it's the verb used in the IL opcodes, and to my mind they key point is that these properties track which IL opcodes get used and how often, so that you can take advantage of them when importing that single IL operation... I don't think that "write" conveys that (though I'll admit that "store" doesn't do a great job of conveying it). Maybe something along the lines of lvSingleILDef or lvSingleILStoreOp?

I could resolve this by setting lvArgWrite when the arg temp is allocated and update the logic for args and then the fields would have same meaning. Does that seem preferable?

maybe? On the one hand, I feel ok with what you have (modulo naming). On the other hand, I had to read over the change a few times to figure out that the asymmetry was intentional and because of the implicit arg def at the callsite... I guess whatever fits with the names is preferable to me, so if you keep them with names matching the opcodes then like you have it seems preferable, but if you switch to something more generic like Def/Write then yeah, reworking it to count the implicit arg defs at callsites as defs/writes would make more sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

Bringing in IL seems like a good improvement, so lvSingleILStoreOp and lvMultipleILStoreOp? And then similarly for the local bools that get introduced....

Copy link

Choose a reason for hiding this comment

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

so lvSingleILStoreOp and lvMultipleILStoreOp?

SGTM

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, lvSingleILStoreOp is sounding more precise than it is, it does not mean exactly one. Maybe lvHasILStoreOp and lvHasMultipleILStoreOp ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also am going to split the Set/Update method into two methods instead of passing in a bool.

Copy link

Choose a reason for hiding this comment

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

Maybe lvHasILStoreOp and lvHasMultipleILStoreOp ?

Sure.

unsigned char lvIsTemp : 1; // Short-lifetime compiler temp
#if OPT_BOOL_OPS
unsigned char lvIsBoolean : 1; // set if variable is boolean
#endif
Expand Down Expand Up @@ -2672,11 +2673,14 @@ class Compiler
// Returns true if this local var is a multireg struct
bool lvaIsMultiregStruct(LclVarDsc* varDsc);

// If the class is a TYP_STRUCT, get/set a class handle describing it

// If the local is a TYP_STRUCT, get/set a class handle describing it
CORINFO_CLASS_HANDLE lvaGetStruct(unsigned varNum);
void lvaSetStruct(unsigned varNum, CORINFO_CLASS_HANDLE typeHnd, bool unsafeValueClsCheck, bool setTypeInfo = true);

// If the local is TYP_REF, set or improve the associated class information.
void lvaSetClass(unsigned varNum, CORINFO_CLASS_HANDLE clsHnd, bool isExact = false);
void lvaSetClass(unsigned varNum, GenTreePtr tree, CORINFO_CLASS_HANDLE stackHandle = nullptr);

#define MAX_NumOfFieldsInPromotableStruct 4 // Maximum number of fields in promotable struct

// Info about struct fields
Expand Down
150 changes: 92 additions & 58 deletions src/jit/flowgraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4552,13 +4552,68 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, BYTE*
// be handled properly by the verifier.
if (varNum < lvaTableCnt)
{
// In non-inline cases, note written-to locals.
// In non-inline cases, note written-to arguments.
lvaTable[varNum].lvArgWrite = 1;
}
}
}
break;

case CEE_STLOC_0:
case CEE_STLOC_1:
case CEE_STLOC_2:
case CEE_STLOC_3:
varNum = (opcode - CEE_STLOC_0);
goto STLOC;

case CEE_STLOC:
case CEE_STLOC_S:
{
noway_assert(sz == sizeof(BYTE) || sz == sizeof(WORD));

if (codeAddr > codeEndp - sz)
{
goto TOO_FAR;
}

varNum = (sz == sizeof(BYTE)) ? getU1LittleEndian(codeAddr) : getU2LittleEndian(codeAddr);

STLOC:
if (isInlining)
{
InlLclVarInfo& lclInfo = impInlineInfo->lclVarInfo[varNum + impInlineInfo->argCnt];

if (lclInfo.lclHasStlocOp)
{
lclInfo.lclHasMultipleStlocOp = 1;
}
else
{
lclInfo.lclHasStlocOp = 1;
}
}
else
{
varNum += info.compArgsCount;

// This check is only intended to prevent an AV. Bad varNum values will later
// be handled properly by the verifier.
if (varNum < lvaTableCnt)
{
// In non-inline cases, note written-to locals.
if (lvaTable[varNum].lvArgWrite)
{
lvaTable[varNum].lvMultipleArgWrite = 1;
}
else
{
lvaTable[varNum].lvArgWrite = 1;
}
}
}
}
break;

case CEE_LDARGA:
case CEE_LDARGA_S:
case CEE_LDLOCA:
Expand Down Expand Up @@ -22331,9 +22386,13 @@ GenTreePtr Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo)

for (unsigned argNum = 0; argNum < inlineInfo->argCnt; argNum++)
{
if (inlArgInfo[argNum].argHasTmp)
const InlArgInfo& argInfo = inlArgInfo[argNum];
const bool argIsSingleDef = !argInfo.argHasLdargaOp && !argInfo.argHasStargOp;
GenTree* const argNode = inlArgInfo[argNum].argNode;
Copy link

Choose a reason for hiding this comment

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

Shorten to argInfo.argNode?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, not sure how I missed these. Will clean them up.


if (argInfo.argHasTmp)
{
noway_assert(inlArgInfo[argNum].argIsUsed);
noway_assert(argInfo.argIsUsed);

/* argBashTmpNode is non-NULL iff the argument's value was
referenced exactly once by the original IL. This offers an
Expand All @@ -22347,14 +22406,12 @@ GenTreePtr Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo)
once) but the optimization cannot be applied.
*/

GenTreePtr argSingleUseNode = inlArgInfo[argNum].argBashTmpNode;
GenTreePtr argSingleUseNode = argInfo.argBashTmpNode;

if (argSingleUseNode && !(argSingleUseNode->gtFlags & GTF_VAR_CLONED) &&
!inlArgInfo[argNum].argHasLdargaOp && !inlArgInfo[argNum].argHasStargOp)
if ((argSingleUseNode != nullptr) && !(argSingleUseNode->gtFlags & GTF_VAR_CLONED) && argIsSingleDef)
{
// Change the temp in-place to the actual argument.
// We currently do not support this for struct arguments, so it must not be a GT_OBJ.
GenTree* argNode = inlArgInfo[argNum].argNode;
assert(argNode->gtOper != GT_OBJ);
argSingleUseNode->CopyFrom(argNode, this);
continue;
Expand All @@ -22363,53 +22420,32 @@ GenTreePtr Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo)
{
// We're going to assign the argument value to the
// temp we use for it in the inline body.
//
// If we know the argument's value can't be
// changed within the method body, try and improve
// the type of the temp.
if (!inlArgInfo[argNum].argHasLdargaOp && !inlArgInfo[argNum].argHasStargOp)
{
GenTree* argNode = inlArgInfo[argNum].argNode;
bool isExact = false;
bool isNonNull = false;
CORINFO_CLASS_HANDLE refClassHandle = gtGetClassHandle(argNode, &isExact, &isNonNull);

if (refClassHandle != nullptr)
{
const unsigned tmpNum = inlArgInfo[argNum].argTmpNum;

// If we already had an exact type for
// this temp, this new information had
// better agree with what we knew before.
if (lvaTable[tmpNum].lvClassIsExact)
{
assert(isExact);
assert(refClassHandle == lvaTable[tmpNum].lvClassHnd);
}
else
{
lvaTable[tmpNum].lvClassHnd = refClassHandle;
lvaTable[tmpNum].lvClassIsExact = isExact;
}
}
}

/* Create the temp assignment for this argument */
const unsigned tmpNum = argInfo.argTmpNum;
const var_types argType = lclVarInfo[argNum].lclTypeInfo;

// Create the temp assignment for this argument
CORINFO_CLASS_HANDLE structHnd = DUMMY_INIT(0);

if (varTypeIsStruct(lclVarInfo[argNum].lclTypeInfo))
if (varTypeIsStruct(argType))
{
structHnd = gtGetStructHandleIfPresent(inlArgInfo[argNum].argNode);
structHnd = gtGetStructHandleIfPresent(argNode);
noway_assert(structHnd != NO_CLASS_HANDLE);
}

// Unsafe value cls check is not needed for
// argTmpNum here since in-linee compiler instance
// would have iterated over these and marked them
// accordingly.
impAssignTempGen(inlArgInfo[argNum].argTmpNum, inlArgInfo[argNum].argNode, structHnd,
(unsigned)CHECK_SPILL_NONE, &afterStmt, callILOffset, block);
impAssignTempGen(tmpNum, argNode, structHnd, (unsigned)CHECK_SPILL_NONE, &afterStmt, callILOffset,
block);

// If we know the argument's value can't be
// changed within the method body, try and improve
// the type of the temp.
if (argIsSingleDef && (argType == TYP_REF))
{
lvaSetClass(tmpNum, argNode);
}

#ifdef DEBUG
if (verbose)
Expand All @@ -22419,44 +22455,42 @@ GenTreePtr Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo)
#endif // DEBUG
}
}
else if (inlArgInfo[argNum].argIsByRefToStructLocal)
else if (argInfo.argIsByRefToStructLocal)
{
// Do nothing.
// Do nothing. Arg was directly substituted as we read
// the inlinee.
}
else
{
/* The argument is either not used or a const or lcl var */

noway_assert(!inlArgInfo[argNum].argIsUsed || inlArgInfo[argNum].argIsInvariant ||
inlArgInfo[argNum].argIsLclVar);
noway_assert(!argInfo.argIsUsed || argInfo.argIsInvariant || argInfo.argIsLclVar);

/* Make sure we didnt change argNode's along the way, or else
subsequent uses of the arg would have worked with the bashed value */
if (inlArgInfo[argNum].argIsInvariant)
if (argInfo.argIsInvariant)
{
assert(inlArgInfo[argNum].argNode->OperIsConst() || inlArgInfo[argNum].argNode->gtOper == GT_ADDR);
assert(argInfo.argNode->OperIsConst() || argInfo.argNode->gtOper == GT_ADDR);
Copy link

Choose a reason for hiding this comment

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

use your new local argNode here?

}
noway_assert((inlArgInfo[argNum].argIsLclVar == 0) ==
(inlArgInfo[argNum].argNode->gtOper != GT_LCL_VAR ||
(inlArgInfo[argNum].argNode->gtFlags & GTF_GLOB_REF)));
noway_assert((argInfo.argIsLclVar == 0) ==
(argInfo.argNode->gtOper != GT_LCL_VAR || (argInfo.argNode->gtFlags & GTF_GLOB_REF)));
Copy link

Choose a reason for hiding this comment

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

argNode?


/* If the argument has side effects, append it */

if (inlArgInfo[argNum].argHasSideEff)
if (argInfo.argHasSideEff)
{
noway_assert(inlArgInfo[argNum].argIsUsed == false);
noway_assert(argInfo.argIsUsed == false);

if (inlArgInfo[argNum].argNode->gtOper == GT_OBJ ||
inlArgInfo[argNum].argNode->gtOper == GT_MKREFANY)
if (argInfo.argNode->gtOper == GT_OBJ || argInfo.argNode->gtOper == GT_MKREFANY)
Copy link

Choose a reason for hiding this comment

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

argNode? (and again once in each branch of the if/else in the next 10 lines)

{
// Don't put GT_OBJ node under a GT_COMMA.
// Codegen can't deal with it.
// Just hang the address here in case there are side-effect.
newStmt = gtNewStmt(gtUnusedValNode(inlArgInfo[argNum].argNode->gtOp.gtOp1), callILOffset);
newStmt = gtNewStmt(gtUnusedValNode(argInfo.argNode->gtOp.gtOp1), callILOffset);
}
else
{
newStmt = gtNewStmt(gtUnusedValNode(inlArgInfo[argNum].argNode), callILOffset);
newStmt = gtNewStmt(gtUnusedValNode(argInfo.argNode), callILOffset);
}
afterStmt = fgInsertStmtAfter(block, afterStmt, newStmt);

Expand Down
Loading