Skip to content

Commit

Permalink
Delete impCheckForNullPointer (#61576)
Browse files Browse the repository at this point in the history
The comment above the method mentions "many problems" with leaving
null pointers around, but it is unclear what kind of problems. I can
only speculate those were the problems in legacy codegen which "could
not handle constant op1".

It also mentions that "we cannot even fold (null+offset)", which is
incorrect: "gtFoldExprConst" does in fact fold such expressions to
zero byrefs. It is also the case that spilling the null into a local
affects little as local assertion propagation happily propagates the
null back into its original place.

There was also a little bug associated with the method that got fixed:
morph was trying to use it, and in the process created uses of a local
that was not initialized, since the statement list used by the method
is the importer's one, invalid in morph.
  • Loading branch information
SingleAccretion authored Nov 15, 2021
1 parent 5d3e70a commit 2e97ac4
Show file tree
Hide file tree
Showing 4 changed files with 2 additions and 66 deletions.
1 change: 0 additions & 1 deletion src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -4610,7 +4610,6 @@ class Compiler

unsigned impInitBlockLineInfo();

GenTree* impCheckForNullPointer(GenTree* obj);
bool impIsThis(GenTree* obj);
bool impIsLDFTN_TOKEN(const BYTE* delegateCreateStart, const BYTE* newobjCodeAddr);
bool impIsDUP_LDVIRTFTN_TOKEN(const BYTE* delegateCreateStart, const BYTE* newobjCodeAddr);
Expand Down
53 changes: 0 additions & 53 deletions src/coreclr/jit/compiler.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -3765,59 +3765,6 @@ inline bool Compiler::compIsProfilerHookNeeded()
#endif // !PROFILING_SUPPORTED
}

/*****************************************************************************
*
* Check for the special case where the object is the constant 0.
* As we can't even fold the tree (null+fldOffs), we are left with
* op1 and op2 both being a constant. This causes lots of problems.
* We simply grab a temp and assign 0 to it and use it in place of the NULL.
*/

inline GenTree* Compiler::impCheckForNullPointer(GenTree* obj)
{
/* If it is not a GC type, we will be able to fold it.
So don't need to do anything */

if (!varTypeIsGC(obj->TypeGet()))
{
return obj;
}

if (obj->gtOper == GT_CNS_INT)
{
assert(obj->gtType == TYP_REF || obj->gtType == TYP_BYREF);

// We can see non-zero byrefs for RVA statics or for frozen strings.
if (obj->AsIntCon()->gtIconVal != 0)
{
#ifdef DEBUG
if (!obj->TypeIs(TYP_BYREF))
{
assert(obj->TypeIs(TYP_REF));
assert(obj->IsIconHandle(GTF_ICON_STR_HDL));
if (!doesMethodHaveFrozenString())
{
assert(compIsForInlining());
assert(impInlineInfo->InlinerCompiler->doesMethodHaveFrozenString());
}
}
#endif // DEBUG
return obj;
}

unsigned tmp = lvaGrabTemp(true DEBUGARG("CheckForNullPointer"));

// We don't need to spill while appending as we are only assigning
// NULL to a freshly-grabbed temp.

impAssignTempGen(tmp, obj, (unsigned)CHECK_SPILL_NONE);

obj = gtNewLclvNode(tmp, obj->gtType);
}

return obj;
}

/*****************************************************************************
*
* Check for the special case where the object is the methods original 'this' pointer.
Expand Down
8 changes: 0 additions & 8 deletions src/coreclr/jit/importer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12794,8 +12794,6 @@ void Compiler::impImportBlockCode(BasicBlock* block)
}
}

op1 = impCheckForNullPointer(op1);

/* Mark the block as containing an index expression */

if (op1->gtOper == GT_LCL_VAR)
Expand Down Expand Up @@ -13009,8 +13007,6 @@ void Compiler::impImportBlockCode(BasicBlock* block)
op2->gtType = TYP_I_IMPL;
}

op3 = impCheckForNullPointer(op3);

// Mark the block as containing an index expression

if (op3->gtOper == GT_LCL_VAR)
Expand Down Expand Up @@ -15220,8 +15216,6 @@ void Compiler::impImportBlockCode(BasicBlock* block)
case CORINFO_FIELD_INSTANCE_WITH_BASE:
#endif
{
obj = impCheckForNullPointer(obj);

// If the object is a struct, what we really want is
// for the field to operate on the address of the struct.
if (!varTypeGCtype(obj->TypeGet()) && impIsValueType(tiObj))
Expand Down Expand Up @@ -15550,8 +15544,6 @@ void Compiler::impImportBlockCode(BasicBlock* block)
case CORINFO_FIELD_INSTANCE_WITH_BASE:
#endif
{
obj = impCheckForNullPointer(obj);

/* Create the data member node */
op1 = gtNewFieldRef(lclTyp, resolvedToken.hField, obj, fieldInfo.offset);
DWORD typeFlags = info.compCompHnd->getClassAttribs(resolvedToken.hClass);
Expand Down
6 changes: 2 additions & 4 deletions src/coreclr/jit/morph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9278,10 +9278,8 @@ GenTree* Compiler::fgMorphCall(GenTreeCall* call)
fgWalkTreePost(&value, resetMorphedFlag);
#endif // DEBUG

GenTree* const nullCheckedArr = impCheckForNullPointer(arr);
GenTree* const arrIndexNode = gtNewIndexRef(TYP_REF, nullCheckedArr, index);
GenTree* const arrStore = gtNewAssignNode(arrIndexNode, value);
arrStore->gtFlags |= GTF_ASG;
GenTree* const arrIndexNode = gtNewIndexRef(TYP_REF, arr, index);
GenTree* const arrStore = gtNewAssignNode(arrIndexNode, value);

GenTree* result = fgMorphTree(arrStore);
if (argSetup != nullptr)
Expand Down

0 comments on commit 2e97ac4

Please sign in to comment.