Delete impCheckForNullPointer
#61576
Conversation
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.
ghost
added
the
community-contribution
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsThe 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. One diff in the tests: MinOpts method no longer had the local spilled. It is expected we will not see a lot of diffs because of the aforementioned local assertion propagation (there are 6 additional diffs with text-only diffs which showed the local in question as
|
| @@ -9236,10 +9236,8 @@ GenTree* Compiler::fgMorphCall(GenTreeCall* call) | |||
| fgWalkTreePost(&value, resetMorphedFlag); | |||
| #endif // DEBUG | |||
|
|
|||
| GenTree* const nullCheckedArr = impCheckForNullPointer(arr); | |||
There was a problem hiding this comment.
This is the morph bug: it was creating an ASG in a statement that did not get properly prepended.
| GenTree* const nullCheckedArr = impCheckForNullPointer(arr); | ||
| GenTree* const arrIndexNode = gtNewIndexRef(TYP_REF, nullCheckedArr, index); | ||
| GenTree* const arrStore = gtNewAssignNode(arrIndexNode, value); | ||
| arrStore->gtFlags |= GTF_ASG; |
There was a problem hiding this comment.
gtNewAssignNode already marks the node with the flag.
|
@dotnet/jit-contrib |
ghost
locked as resolved and limited conversation to collaborators
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.
One diff in the tests: MinOpts method no longer had the local spilled. It is expected we will not see a lot of diffs because of the aforementioned local assertion propagation (there are 6 additional text-only diffs which showed the local in question as
zero-ref, as we'd expect).