Don't remove dead indirections that may throw.

[erozenfeld]

Liveness incorrectly removed indirection rhs's of dead assignments.
This change fixes that.

Fixes #39823.

[erozenfeld]

x64 pmi framework diffs:

PMI CodeSize Diffs for System.Private.CoreLib.dll, framework assemblies for  default jit
Summary of Code Size diffs:
(Lower is better)
Total bytes of diff: 28 (0.00% of base)
    diff is a regression.
Top file regressions (bytes):
          10 : System.Private.Xml.dasm (0.00% of base)
           9 : System.Private.CoreLib.dasm (0.00% of base)
           6 : System.Collections.dasm (0.00% of base)
           3 : System.Reflection.Metadata.dasm (0.00% of base)
4 total files with Code Size differences (0 improved, 4 regressed), 263 unchanged.
Top method regressions (bytes):
          10 ( 1.16% of base) : System.Private.Xml.dasm - <ParseTextAsync_AsyncFunc>d__543:MoveNext():this
           7 ( 1.02% of base) : System.Private.CoreLib.dasm - Vector256`1:Equals(Object):bool:this (6 methods)
           6 ( 5.56% of base) : System.Collections.dasm - <>c__DisplayClass34_0:<ContainsValue>b__0(Node):bool:this (7 methods)
           3 ( 1.68% of base) : System.Reflection.Metadata.dasm - PEHeaders:GetContainingSectionIndex(int):int:this
           2 ( 1.28% of base) : System.Private.CoreLib.dasm - Vector256`1:Equals(Vector256`1):bool:this (6 methods)
Top method regressions (percentages):
           6 ( 5.56% of base) : System.Collections.dasm - <>c__DisplayClass34_0:<ContainsValue>b__0(Node):bool:this (7 methods)
           3 ( 1.68% of base) : System.Reflection.Metadata.dasm - PEHeaders:GetContainingSectionIndex(int):int:this
           2 ( 1.28% of base) : System.Private.CoreLib.dasm - Vector256`1:Equals(Vector256`1):bool:this (6 methods)
          10 ( 1.16% of base) : System.Private.Xml.dasm - <ParseTextAsync_AsyncFunc>d__543:MoveNext():this
           7 ( 1.02% of base) : System.Private.CoreLib.dasm - Vector256`1:Equals(Object):bool:this (6 methods)
5 total methods with Code Size differences (0 improved, 5 regressed), 253493 unchanged.

[erozenfeld]

No x64 pmi benchmark diffs.

[erozenfeld]

@sandreenko @dotnet/jit-contrib PTAL

[sandreenko]

LGTM

[erozenfeld]

@sandreenko I tweaked my fix so that the representation of unconsumed indirections is consistent. The diffs are the same as with the previous version of my changes. PTAL.

[sandreenko]

LGTM, I will use this function to fix follow up for https://github.com/dotnet/runtime/pull/39824/files

[CarolEidt]

I think that the logic for when to generate GT_IND vs. GT_NULLCHECK shoud remain in Lowering - see Lowering::LowerIndir(). Also, I found that if I changed dead indirections to GT_NULLCHECK too early in the compile, CSE would miss out on opportunities to CSE dead indirections in loops.

[erozenfeld]

I need to call this function from liveness that’s running during lower, that’s why I placed on Compiler. If I kept it in Lower, I would have to pass an instance of Lower to liveness, which would be odd.

[sandreenko]

@CarolEidt it appeared that these unused IND could appear after lowering so we can't rely on LowerIndir to replace NULLCHECK with IND for arm32.

[CarolEidt]

I see. That said, I still think that we should 1) avoid generating GT_NULLCHECK when not in LIR form, and 2) leave the method on Lowering that determines whether to transform to GT_NULLCHECK as it is target-dependent. It can be static, and then this method can do the actual transformation, since it updates block and Compiler state (that said, it seems to me that if we care about whether the method or block has a nullcheck, that should be set for the GT_IND form of nullcheck as well). As much as possible, I'd like to keep the target-dependent code in the backend files.

[sandreenko]

Also, I found that if I changed dead indirections to GT_NULLCHECK too early in the compile, CSE would miss out on opportunities to CSE dead indirections in loops.

we have optimizations in earlyProp that prefer NULLCHECK over IND, see OMF_HAS_NULLCHECK. So if we stop generating NULLCHECK before lowering we would get both regressions and improvements.

Also, the importer generates null checks in some cases instead of GT_IND and GT_FIELD.

I agree that it is error-prone and confusing, would be nice to find a better way to handle this.

[erozenfeld]

1. We intentionally generate GT_NULLCHECK before LIR in many places because both earlyProp (gtFoldNullCheck) and assertion propagation have optimizations to eliminate redundant null checks.
2. I can make the method static on Lower and pass Compiler and block instances to it. Would that address your concern?

[erozenfeld]

Currently the nullcheck flags on the compiler instance and basic block are only used by earlyProp. Still, I'd like to keep them up-to-date.

[CarolEidt]

@erozenfeld - that would be fine, but it would also be fine to have a static method on Lowering that simply determined which option to use.

[erozenfeld]

@CarolEidt I addressed your feedback. PTAL.

[CarolEidt]

LGTM - thanks