Skip to content

Commit 2e97ac4

Browse files
Delete impCheckForNullPointer (#61576)
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.
1 parent 5d3e70a commit 2e97ac4

File tree

4 files changed

+2
-66
lines changed

4 files changed

+2
-66
lines changed

src/coreclr/jit/compiler.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4610,7 +4610,6 @@ class Compiler
46104610

46114611
unsigned impInitBlockLineInfo();
46124612

4613-
GenTree* impCheckForNullPointer(GenTree* obj);
46144613
bool impIsThis(GenTree* obj);
46154614
bool impIsLDFTN_TOKEN(const BYTE* delegateCreateStart, const BYTE* newobjCodeAddr);
46164615
bool impIsDUP_LDVIRTFTN_TOKEN(const BYTE* delegateCreateStart, const BYTE* newobjCodeAddr);

src/coreclr/jit/compiler.hpp

Lines changed: 0 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -3765,59 +3765,6 @@ inline bool Compiler::compIsProfilerHookNeeded()
37653765
#endif // !PROFILING_SUPPORTED
37663766
}
37673767

3768-
/*****************************************************************************
3769-
*
3770-
* Check for the special case where the object is the constant 0.
3771-
* As we can't even fold the tree (null+fldOffs), we are left with
3772-
* op1 and op2 both being a constant. This causes lots of problems.
3773-
* We simply grab a temp and assign 0 to it and use it in place of the NULL.
3774-
*/
3775-
3776-
inline GenTree* Compiler::impCheckForNullPointer(GenTree* obj)
3777-
{
3778-
/* If it is not a GC type, we will be able to fold it.
3779-
So don't need to do anything */
3780-
3781-
if (!varTypeIsGC(obj->TypeGet()))
3782-
{
3783-
return obj;
3784-
}
3785-
3786-
if (obj->gtOper == GT_CNS_INT)
3787-
{
3788-
assert(obj->gtType == TYP_REF || obj->gtType == TYP_BYREF);
3789-
3790-
// We can see non-zero byrefs for RVA statics or for frozen strings.
3791-
if (obj->AsIntCon()->gtIconVal != 0)
3792-
{
3793-
#ifdef DEBUG
3794-
if (!obj->TypeIs(TYP_BYREF))
3795-
{
3796-
assert(obj->TypeIs(TYP_REF));
3797-
assert(obj->IsIconHandle(GTF_ICON_STR_HDL));
3798-
if (!doesMethodHaveFrozenString())
3799-
{
3800-
assert(compIsForInlining());
3801-
assert(impInlineInfo->InlinerCompiler->doesMethodHaveFrozenString());
3802-
}
3803-
}
3804-
#endif // DEBUG
3805-
return obj;
3806-
}
3807-
3808-
unsigned tmp = lvaGrabTemp(true DEBUGARG("CheckForNullPointer"));
3809-
3810-
// We don't need to spill while appending as we are only assigning
3811-
// NULL to a freshly-grabbed temp.
3812-
3813-
impAssignTempGen(tmp, obj, (unsigned)CHECK_SPILL_NONE);
3814-
3815-
obj = gtNewLclvNode(tmp, obj->gtType);
3816-
}
3817-
3818-
return obj;
3819-
}
3820-
38213768
/*****************************************************************************
38223769
*
38233770
* Check for the special case where the object is the methods original 'this' pointer.

src/coreclr/jit/importer.cpp

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12794,8 +12794,6 @@ void Compiler::impImportBlockCode(BasicBlock* block)
1279412794
}
1279512795
}
1279612796

12797-
op1 = impCheckForNullPointer(op1);
12798-
1279912797
/* Mark the block as containing an index expression */
1280012798

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

13012-
op3 = impCheckForNullPointer(op3);
13013-
1301413010
// Mark the block as containing an index expression
1301513011

1301613012
if (op3->gtOper == GT_LCL_VAR)
@@ -15220,8 +15216,6 @@ void Compiler::impImportBlockCode(BasicBlock* block)
1522015216
case CORINFO_FIELD_INSTANCE_WITH_BASE:
1522115217
#endif
1522215218
{
15223-
obj = impCheckForNullPointer(obj);
15224-
1522515219
// If the object is a struct, what we really want is
1522615220
// for the field to operate on the address of the struct.
1522715221
if (!varTypeGCtype(obj->TypeGet()) && impIsValueType(tiObj))
@@ -15550,8 +15544,6 @@ void Compiler::impImportBlockCode(BasicBlock* block)
1555015544
case CORINFO_FIELD_INSTANCE_WITH_BASE:
1555115545
#endif
1555215546
{
15553-
obj = impCheckForNullPointer(obj);
15554-
1555515547
/* Create the data member node */
1555615548
op1 = gtNewFieldRef(lclTyp, resolvedToken.hField, obj, fieldInfo.offset);
1555715549
DWORD typeFlags = info.compCompHnd->getClassAttribs(resolvedToken.hClass);

src/coreclr/jit/morph.cpp

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9278,10 +9278,8 @@ GenTree* Compiler::fgMorphCall(GenTreeCall* call)
92789278
fgWalkTreePost(&value, resetMorphedFlag);
92799279
#endif // DEBUG
92809280

9281-
GenTree* const nullCheckedArr = impCheckForNullPointer(arr);
9282-
GenTree* const arrIndexNode = gtNewIndexRef(TYP_REF, nullCheckedArr, index);
9283-
GenTree* const arrStore = gtNewAssignNode(arrIndexNode, value);
9284-
arrStore->gtFlags |= GTF_ASG;
9281+
GenTree* const arrIndexNode = gtNewIndexRef(TYP_REF, arr, index);
9282+
GenTree* const arrStore = gtNewAssignNode(arrIndexNode, value);
92859283

92869284
GenTree* result = fgMorphTree(arrStore);
92879285
if (argSetup != nullptr)

0 commit comments

Comments
 (0)