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

Change gtGetThisArg not to return nullptr. #44398

Merged
merged 3 commits into from
Nov 19, 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
20 changes: 9 additions & 11 deletions src/coreclr/src/jit/assertionprop.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2163,22 +2163,20 @@ void Compiler::optAssertionGen(GenTree* tree)
break;

case GT_CALL:
{
// A virtual call can create a non-null assertion. We transform some virtual calls into non-virtual calls
// with a GTF_CALL_NULLCHECK flag set.
if ((tree->gtFlags & GTF_CALL_NULLCHECK) || tree->AsCall()->IsVirtual())
// Ignore tail calls because they have 'this` pointer in the regular arg list and an implicit null check.
GenTreeCall* const call = tree->AsCall();
if (call->NeedsNullCheck() || (call->IsVirtual() && !call->IsTailCall()))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fgMorphTailCallViaJitHelper we clear NeedsNullCheck, should not we also clear IsVirtual?
Then this condition will be just if (call->NeedsNullCheck() || call->IsVirtual())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jakobbotsch could you please advise me here?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I don't know. fgMorphTailCallViaJitHelper is the old (from before #341) tailcall mechanism used only on x86. This mechanism might be used directly for virtual calls (or VSDs), but I don't remember how the address is computed in those cases.

{
// Retrieve the 'this' arg
GenTree* thisArg = gtGetThisArg(tree->AsCall());
if (thisArg == nullptr)
{
// For tail calls we lose the this pointer in the argument list but that's OK because a null check
// was made explicit, so we get the assertion when we walk the GT_IND in the argument list.
noway_assert(tree->AsCall()->IsTailCall());
break;
}
// Retrieve the 'this' arg.
GenTree* thisArg = gtGetThisArg(call);
assert(thisArg != nullptr);
assertionInfo = optCreateAssertion(thisArg, nullptr, OAK_NOT_EQUAL);
}
break;
}
break;

case GT_CAST:
// We only create this assertion for global assertion prop
Expand Down
68 changes: 35 additions & 33 deletions src/coreclr/src/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6082,7 +6082,6 @@ GenTree* Compiler::gtNewStringLiteralNode(InfoAccessType iat, void* pValue)
#ifdef DEBUG
tree->AsIntCon()->gtTargetHandle = (size_t)pValue;
#endif
tree = gtNewOperNode(GT_NOP, TYP_REF, tree); // prevents constant folding
break;

case IAT_PVALUE: // The value needs to be accessed via an indirection
Expand Down Expand Up @@ -8535,57 +8534,60 @@ bool Compiler::gtCompareTree(GenTree* op1, GenTree* op2)
return false;
}

//------------------------------------------------------------------------
// gtGetThisArg: Return this pointer node for the call.
//
// Arguments:
// call - the call node with a this argument.
//
// Return value:
// the this pointer node.
//
GenTree* Compiler::gtGetThisArg(GenTreeCall* call)
{
if (call->gtCallThisArg == nullptr)
{
return nullptr;
}
assert(call->gtCallThisArg != nullptr);

GenTree* thisArg = call->gtCallThisArg->GetNode();
if (thisArg->OperIs(GT_NOP, GT_ASG) == false)
if (!thisArg->OperIs(GT_ASG))
{
if ((thisArg->gtFlags & GTF_LATE_ARG) == 0)
{
return thisArg;
}
}

if (call->gtCallLateArgs != nullptr)
{
unsigned argNum = 0;
fgArgTabEntry* thisArgTabEntry = gtArgEntryByArgNum(call, argNum);
GenTree* result = thisArgTabEntry->GetNode();
assert(call->gtCallLateArgs != nullptr);

// Assert if we used DEBUG_DESTROY_NODE.
assert(result->gtOper != GT_COUNT);
unsigned argNum = 0;
fgArgTabEntry* thisArgTabEntry = gtArgEntryByArgNum(call, argNum);
GenTree* result = thisArgTabEntry->GetNode();

// Assert if we used DEBUG_DESTROY_NODE.
assert(result->gtOper != GT_COUNT);

#if !FEATURE_FIXED_OUT_ARGS && defined(DEBUG)
// Check that call->fgArgInfo used in gtArgEntryByArgNum was not
// left outdated by assertion propogation updates.
// There is no information about registers of late args for platforms
// with FEATURE_FIXED_OUT_ARGS that is why this debug check is under
// !FEATURE_FIXED_OUT_ARGS.
regNumber thisReg = REG_ARG_0;
regList list = call->regArgList;
int index = 0;
for (GenTreeCall::Use& use : call->LateArgs())
// Check that call->fgArgInfo used in gtArgEntryByArgNum was not
// left outdated by assertion propogation updates.
// There is no information about registers of late args for platforms
// with FEATURE_FIXED_OUT_ARGS that is why this debug check is under
// !FEATURE_FIXED_OUT_ARGS.
regNumber thisReg = REG_ARG_0;
regList list = call->regArgList;
int index = 0;
for (GenTreeCall::Use& use : call->LateArgs())
{
assert(index < call->regArgListCount);
regNumber curArgReg = list[index];
if (curArgReg == thisReg)
{
assert(index < call->regArgListCount);
regNumber curArgReg = list[index];
if (curArgReg == thisReg)
{
assert(result == use.GetNode());
}

index++;
assert(result == use.GetNode());
}
#endif // !FEATURE_FIXED_OUT_ARGS && defined(DEBUG)

return result;
index++;
}
#endif // !FEATURE_FIXED_OUT_ARGS && defined(DEBUG)

return nullptr;
return result;
}

bool GenTree::gtSetFlags() const
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/src/jit/lower.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3762,6 +3762,7 @@ GenTree* Lowering::LowerDelegateInvoke(GenTreeCall* call)
thisArgNode = comp->gtGetThisArg(call);
}

assert(thisArgNode != nullptr);
assert(thisArgNode->gtOper == GT_PUTARG_REG);
GenTree* originalThisExpr = thisArgNode->AsOp()->gtOp1;
GenTree* thisExpr = originalThisExpr;
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/src/jit/morph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8435,6 +8435,10 @@ void Compiler::fgMorphTailCallViaJitHelper(GenTreeCall* call)
thisPtr = objp;
}

// TODO-Cleanup: we leave it as a virtual stub call to
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a try to clear GTF_CALL_VIRT_KIND_MASK here but it caused failures because logic in LowerCall depends on this flag to be set in some cases, so I reverted that change.

It would be nice to clear it and fix the failures but it is out of the scope of this bug fix for CoreRT.

// use logic in `LowerVirtualStubCall`, clear GTF_CALL_VIRT_KIND_MASK here
// and change `LowerCall` to recognize it as a direct call.

// During rationalization tmp="this" and null check will
// materialize as embedded stmts in right execution order.
assert(thisPtr != nullptr);
Expand Down