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
community-contribution
|
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
|
| 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
3 participants
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).