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

Commit

Permalink
Make GT_LIST processing non-recursive to avoid StackOverflow.
Browse files Browse the repository at this point in the history 
We had some internal cases where crossgen failed with StackOverflow exception
when compiling huge methods. In particular, the methods had GT_PHI nodes with
huge number of arguments.

StackOverflow was happening in multiple places. Recent LIR changes eliminated two of those places,
these changes eliminate two more: gtSetEvalOrder and fgDebugCheckFlags (debug only). We already had
gtSetListOrder but it was only used for call arg lists. I made gtSetListOrder non-recursive and also generalized
to handle other GT_LIST nodes.

With that with these changes the huge repros can now be crossgen'd.

I verified no assembly diffs in SuperPMI.

I'm verifying overall throughput effect.
  • Loading branch information
@erozenfeld
erozenfeld committed Sep 7, 2016
1 parent bcd4c0c commit a67f569
Show file tree
Hide file tree
Showing 3 changed files with 282 additions and 157 deletions.
3 changes: 2 additions & 1 deletion src/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1994,7 +1994,7 @@ class Compiler

unsigned gtHashValue(GenTree* tree);

unsigned gtSetListOrder(GenTree* list, bool regs);
unsigned gtSetListOrder(GenTree* list, bool regs, bool isListCallArgs);

void gtWalkOp(GenTree** op1, GenTree** op2, GenTree* adr, bool constOnly);

Expand Down Expand Up @@ -4201,6 +4201,7 @@ class Compiler
void fgDebugCheckLinks(bool morphTrees = false);
void fgDebugCheckNodeLinks(BasicBlock* block, GenTreePtr stmt);
void fgDebugCheckFlags(GenTreePtr tree);
void fgDebugCheckFlagsHelper(GenTreePtr tree, unsigned treeFlags, unsigned chkFlags);
#endif

#ifdef LEGACY_BACKEND
Expand Down
269 changes: 161 additions & 108 deletions src/jit/flowgraph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20239,21 +20239,55 @@ void Compiler::fgDebugCheckFlags(GenTreePtr tree)
}
break;

case GT_LIST:
if ((op2 != nullptr) && op2->IsList())
{
ArrayStack<GenTree *> stack(this);
while ((tree->gtGetOp2() != nullptr) && tree->gtGetOp2()->IsList())
{
stack.Push(tree);
tree = tree->gtGetOp2();
}

fgDebugCheckFlags(tree);

while (stack.Height() > 0)
{
tree = stack.Pop();
assert((tree->gtFlags & GTF_REVERSE_OPS) == 0);
fgDebugCheckFlags(tree->gtOp.gtOp1);
chkFlags |= (tree->gtOp.gtOp1->gtFlags & GTF_ALL_EFFECT);
chkFlags |= (tree->gtGetOp2()->gtFlags & GTF_ALL_EFFECT);
fgDebugCheckFlagsHelper(tree, (tree->gtFlags & GTF_ALL_EFFECT), chkFlags);
}

return;
}
break;

default:
break;
}

/* Recursively check the subtrees */

if (op1) { fgDebugCheckFlags(op1);
}
if (op2) { fgDebugCheckFlags(op2);
}
if (op1)
{
fgDebugCheckFlags(op1);
}
if (op2)
{
fgDebugCheckFlags(op2);
}

if (op1) { chkFlags |= (op1->gtFlags & GTF_ALL_EFFECT);
}
if (op2) { chkFlags |= (op2->gtFlags & GTF_ALL_EFFECT);
}
if (op1)
{
chkFlags |= (op1->gtFlags & GTF_ALL_EFFECT);
}
if (op2)
{
chkFlags |= (op2->gtFlags & GTF_ALL_EFFECT);
}

// We reuse the value of GTF_REVERSE_OPS for a GT_IND-specific flag,
// so exempt that (unary) operator.
Expand Down Expand Up @@ -20315,131 +20349,150 @@ void Compiler::fgDebugCheckFlags(GenTreePtr tree)

/* See what kind of a special operator we have here */

else { switch (tree->OperGet())
{
case GT_CALL:
else
{
switch (tree->OperGet())
{
case GT_CALL:

GenTreePtr args;
GenTreePtr argx;
GenTreeCall* call;
GenTreePtr args;
GenTreePtr argx;
GenTreeCall* call;

call = tree->AsCall();
call = tree->AsCall();

chkFlags |= GTF_CALL;
chkFlags |= GTF_CALL;

if ((treeFlags & GTF_EXCEPT) && !(chkFlags & GTF_EXCEPT))
{
switch (eeGetHelperNum(tree->gtCall.gtCallMethHnd))
if ((treeFlags & GTF_EXCEPT) && !(chkFlags & GTF_EXCEPT))
{
// Is this a helper call that can throw an exception ?
case CORINFO_HELP_LDIV:
case CORINFO_HELP_LMOD:
case CORINFO_HELP_METHOD_ACCESS_CHECK:
case CORINFO_HELP_FIELD_ACCESS_CHECK:
case CORINFO_HELP_CLASS_ACCESS_CHECK:
case CORINFO_HELP_DELEGATE_SECURITY_CHECK:
chkFlags |= GTF_EXCEPT;
break;
default:
break;
switch (eeGetHelperNum(tree->gtCall.gtCallMethHnd))
{
// Is this a helper call that can throw an exception ?
case CORINFO_HELP_LDIV:
case CORINFO_HELP_LMOD:
case CORINFO_HELP_METHOD_ACCESS_CHECK:
case CORINFO_HELP_FIELD_ACCESS_CHECK:
case CORINFO_HELP_CLASS_ACCESS_CHECK:
case CORINFO_HELP_DELEGATE_SECURITY_CHECK:
chkFlags |= GTF_EXCEPT;
break;
default:
break;
}
}
}

if (call->gtCallObjp)
{
fgDebugCheckFlags(call->gtCallObjp);
chkFlags |= (call->gtCallObjp->gtFlags & GTF_SIDE_EFFECT);

if (call->gtCallObjp->gtFlags & GTF_ASG)
if (call->gtCallObjp)
{
treeFlags |= GTF_ASG;
fgDebugCheckFlags(call->gtCallObjp);
chkFlags |= (call->gtCallObjp->gtFlags & GTF_SIDE_EFFECT);

if (call->gtCallObjp->gtFlags & GTF_ASG)
{
treeFlags |= GTF_ASG;
}
}
}

for (args = call->gtCallArgs; args; args = args->gtOp.gtOp2)
{
argx = args->gtOp.gtOp1;
fgDebugCheckFlags(argx);
for (args = call->gtCallArgs; args; args = args->gtOp.gtOp2)
{
argx = args->gtOp.gtOp1;
fgDebugCheckFlags(argx);

chkFlags |= (argx->gtFlags & GTF_SIDE_EFFECT);
chkFlags |= (argx->gtFlags & GTF_SIDE_EFFECT);

if (argx->gtFlags & GTF_ASG)
{
treeFlags |= GTF_ASG;
if (argx->gtFlags & GTF_ASG)
{
treeFlags |= GTF_ASG;
}
}
}

for (args = call->gtCallLateArgs; args; args = args->gtOp.gtOp2)
{
argx = args->gtOp.gtOp1;
fgDebugCheckFlags(argx);
for (args = call->gtCallLateArgs; args; args = args->gtOp.gtOp2)
{
argx = args->gtOp.gtOp1;
fgDebugCheckFlags(argx);

chkFlags |= (argx->gtFlags & GTF_SIDE_EFFECT);
chkFlags |= (argx->gtFlags & GTF_SIDE_EFFECT);

if (argx->gtFlags & GTF_ASG)
{
treeFlags |= GTF_ASG;
if (argx->gtFlags & GTF_ASG)
{
treeFlags |= GTF_ASG;
}
}
}

if ((call->gtCallType == CT_INDIRECT) && (call->gtCallCookie != nullptr))
{
fgDebugCheckFlags(call->gtCallCookie);
chkFlags |= (call->gtCallCookie->gtFlags & GTF_SIDE_EFFECT);
}

if (call->gtCallType == CT_INDIRECT)
{
fgDebugCheckFlags(call->gtCallAddr);
chkFlags |= (call->gtCallAddr->gtFlags & GTF_SIDE_EFFECT);
}
if ((call->gtCallType == CT_INDIRECT) && (call->gtCallCookie != nullptr))
{
fgDebugCheckFlags(call->gtCallCookie);
chkFlags |= (call->gtCallCookie->gtFlags & GTF_SIDE_EFFECT);
}

if (call->IsUnmanaged() &&
(call->gtCallMoreFlags & GTF_CALL_M_UNMGD_THISCALL))
{
if (call->gtCallArgs->gtOp.gtOp1->OperGet() == GT_NOP)
if (call->gtCallType == CT_INDIRECT)
{
noway_assert(call->gtCallLateArgs->gtOp.gtOp1->TypeGet() == TYP_I_IMPL ||
call->gtCallLateArgs->gtOp.gtOp1->TypeGet() == TYP_BYREF);
fgDebugCheckFlags(call->gtCallAddr);
chkFlags |= (call->gtCallAddr->gtFlags & GTF_SIDE_EFFECT);
}
else

if (call->IsUnmanaged() &&
(call->gtCallMoreFlags & GTF_CALL_M_UNMGD_THISCALL))
{
noway_assert(call->gtCallArgs->gtOp.gtOp1->TypeGet() == TYP_I_IMPL ||
call->gtCallArgs->gtOp.gtOp1->TypeGet() == TYP_BYREF);
if (call->gtCallArgs->gtOp.gtOp1->OperGet() == GT_NOP)
{
noway_assert(call->gtCallLateArgs->gtOp.gtOp1->TypeGet() == TYP_I_IMPL ||
call->gtCallLateArgs->gtOp.gtOp1->TypeGet() == TYP_BYREF);
}
else
{
noway_assert(call->gtCallArgs->gtOp.gtOp1->TypeGet() == TYP_I_IMPL ||
call->gtCallArgs->gtOp.gtOp1->TypeGet() == TYP_BYREF);
}
}
}
break;
break;

case GT_ARR_ELEM:
case GT_ARR_ELEM:

GenTreePtr arrObj;
unsigned dim;
GenTreePtr arrObj;
unsigned dim;

arrObj = tree->gtArrElem.gtArrObj;
fgDebugCheckFlags(arrObj);
chkFlags |= (arrObj->gtFlags & GTF_ALL_EFFECT);
arrObj = tree->gtArrElem.gtArrObj;
fgDebugCheckFlags(arrObj);
chkFlags |= (arrObj->gtFlags & GTF_ALL_EFFECT);

for (dim = 0; dim < tree->gtArrElem.gtArrRank; dim++)
{
fgDebugCheckFlags(tree->gtArrElem.gtArrInds[dim]);
chkFlags |= tree->gtArrElem.gtArrInds[dim]->gtFlags & GTF_ALL_EFFECT;
}
break;
for (dim = 0; dim < tree->gtArrElem.gtArrRank; dim++)
{
fgDebugCheckFlags(tree->gtArrElem.gtArrInds[dim]);
chkFlags |= tree->gtArrElem.gtArrInds[dim]->gtFlags & GTF_ALL_EFFECT;
}
break;

case GT_ARR_OFFSET:
fgDebugCheckFlags(tree->gtArrOffs.gtOffset);
chkFlags |= (tree->gtArrOffs.gtOffset->gtFlags & GTF_ALL_EFFECT);
fgDebugCheckFlags(tree->gtArrOffs.gtIndex);
chkFlags |= (tree->gtArrOffs.gtIndex->gtFlags & GTF_ALL_EFFECT);
fgDebugCheckFlags(tree->gtArrOffs.gtArrObj);
chkFlags |= (tree->gtArrOffs.gtArrObj->gtFlags & GTF_ALL_EFFECT);
break;
case GT_ARR_OFFSET:
fgDebugCheckFlags(tree->gtArrOffs.gtOffset);
chkFlags |= (tree->gtArrOffs.gtOffset->gtFlags & GTF_ALL_EFFECT);
fgDebugCheckFlags(tree->gtArrOffs.gtIndex);
chkFlags |= (tree->gtArrOffs.gtIndex->gtFlags & GTF_ALL_EFFECT);
fgDebugCheckFlags(tree->gtArrOffs.gtArrObj);
chkFlags |= (tree->gtArrOffs.gtArrObj->gtFlags & GTF_ALL_EFFECT);
break;

default:
break;
default:
break;
}
}

fgDebugCheckFlagsHelper(tree, treeFlags, chkFlags);
}

//------------------------------------------------------------------------------
// fgDebugCheckFlagsHelper : Check if all bits that are set in chkFlags are also set in treeFlags.
//
//
// Arguments:
// tree - Tree whose flags are being checked
// treeFlags - Actual flags on the tree
// chkFlags - Expected flags
//
// Note:
// Checking that all bits that are set in treeFlags are also set in chkFlags is currently disabled.

void Compiler::fgDebugCheckFlagsHelper(GenTreePtr tree, unsigned treeFlags, unsigned chkFlags)
{
if (chkFlags & ~treeFlags)
{
// Print the tree so we can see it in the log.
Expand All @@ -20461,12 +20514,12 @@ void Compiler::fgDebugCheckFlags(GenTreePtr tree)
#if 0
// TODO-Cleanup:
/* The tree has extra flags set. However, this will happen if we
replace a subtree with something, but don't clear the flags up
the tree. Can't flag this unless we start clearing flags above.
replace a subtree with something, but don't clear the flags up
the tree. Can't flag this unless we start clearing flags above.

Note: we need this working for GTF_CALL and CSEs, so I'm enabling
it for calls.
*/
Note: we need this working for GTF_CALL and CSEs, so I'm enabling
it for calls.
*/
if (tree->OperGet() != GT_CALL && (treeFlags & GTF_CALL) && !(chkFlags & GTF_CALL))
{
// Print the tree so we can see it in the log.
Expand All @@ -20482,9 +20535,9 @@ void Compiler::fgDebugCheckFlags(GenTreePtr tree)
GenTree::gtDispFlags(treeFlags & ~chkFlags, GTF_DEBUG_NONE);
printf("\n");
gtDispTree(tree);
}
#endif // 0
}
#endif // 0
}
}

// DEBUG routine to check correctness of the internal gtNext, gtPrev threading of a statement.
Expand Down
Loading

0 comments on commit a67f569

Please sign in to comment.