Skip to content
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
Delete impCheckForNullPointer
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 committed Nov 14, 2021
commit 4fa791f8421a5bbdf614e73db232ba6983e06590
1 change: 0 additions & 1 deletion src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -4587,7 +4587,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 @@ -3773,59 +3773,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 @@ -12786,8 +12786,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 @@ -13001,8 +12999,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 @@ -15212,8 +15208,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 @@ -15542,8 +15536,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 @@ -9236,10 +9236,8 @@ GenTree* Compiler::fgMorphCall(GenTreeCall* call)
fgWalkTreePost(&value, resetMorphedFlag);
#endif // DEBUG

GenTree* const nullCheckedArr = impCheckForNullPointer(arr);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the morph bug: it was creating an ASG in a statement that did not get properly prepended.

GenTree* const arrIndexNode = gtNewIndexRef(TYP_REF, nullCheckedArr, index);
GenTree* const arrStore = gtNewAssignNode(arrIndexNode, value);
arrStore->gtFlags |= GTF_ASG;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

gtNewAssignNode already marks the node with the flag.

GenTree* const arrIndexNode = gtNewIndexRef(TYP_REF, arr, index);
GenTree* const arrStore = gtNewAssignNode(arrIndexNode, value);

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