JIT: Avoid introducing GT_NULLCHECK in gtTryRemoveBoxUpstreamEffects

[jakobbotsch]

Adding GT_NULLCHECK nodes requires knowledge of which basic block the
nullcheck will end up in. We do not have this easily available in all
places this method is called, and regardless using GT_NULLCHECK here has
minimal diffs.

Alternative we could store the BasicBlock together with GenTreeBox::gtCopyStmtWhenInlinedBoxValue,
but the diffs here are so small that I didn't bother.

Fix #71193

Also an unrelated comment clean-up that I have noticed while looking at early prop (we do not do this optimization here anymore).

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Adding GT_NULLCHECK nodes requires knowledge of which basic block the
nullcheck will end up in. We do not have this easily available in all
places this method is called, and regardless using GT_NULLCHECK here has
minimal diffs.

Alternative we could store the BasicBlock together with GenTreeBox::gtCopyStmtWhenInlinedBoxValue,
but the diffs here are so small that I didn't bother.

Fix #71193

Also an unrelated comment clean-up that I have noticed while looking at early prop (we do not do this optimization here anymore).

Author:	jakobbotsch
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[jakobbotsch]

cc @dotnet/jit-contrib PTAL @AndyAyersMS

[AndyAyersMS]

This is reverting the code back to how I originally wrote it long ago.

Won't we now miss early folding opportunities because we may not see OMF_HAS_NULLCHECK and/or BBF_HAS_NULLCHECK in early prop?

[jakobbotsch]

Won't we now miss early folding opportunities because we may not see OMF_HAS_NULLCHECK and/or BBF_HAS_NULLCHECK in early prop?

Yes, there are a handful of missed opportunities in the diffs, but it is minimal.
FWIW, the function is trying to put BBF_HAS_NULLCHECK on compCurBB which is incorrect when it is used during inlining (global variables strike again...).
Correct marking here requires us to store the BasicBlock in GenTreeBox. But as you can see from the diffs it is not very impactful.

This whole area could do with some unifying. gtTryRemoveBoxUpstreamEffects and gtExtractSideEffList both convert struct indirs into GT_NULLCHECK but leave primitive indirs alone. Top-level indirs are also always just left alone. Leaving primitive indirs alone probably benefits CSE, but this already constitutes lots of missed opportunities in early prop. As I have mentioned offline I think we would be better served with removing GT_NULLCHECK, at least until lowering, and then supporting folding the unused indirs in early prop.

[AndyAyersMS]

I'm fine with the direction you want to head in -- we should also decide if all this opportunity flagging we use to gate early prop is worth the trouble.

[jakobbotsch]

we should also decide if all this opportunity flagging we use to gate early prop is worth the trouble

Yes, this part is also unfortunate. At the very least we could probably use one of the full IR walks that are happening just prior to SSA to mark this information so that we don't need to keep it up to date all the time.