Remove O1K_VALUE_NUMBER

[EgorBo]

I am seeing some nice diffs in libraries.pmi from this change.

My understanding that when we try to create an assertion for lcl != null (where lcl is a TYP_BYREF) - we try to find the underlying TYP_REF in that TYP_BYREF (e.g. BYREF = TYP_REF + smallCns) and then create a O1K_VALUE_NUMBER assertion for that TYP_REF or bail out.

My PR removes that assertion completely so we just create "byref lcl != null" O1K_LCLVAR assertion without trying to find the TYP_REF. There are a few regressions, but mostly improvements.

E.g. it helps to fold things like:

***** BB01 [0000]
STMT00011
N002 (  2,  2) [000080] ---X-O-----                         *  NULLCHECK byte   $181
N001 (  1,  1) [000079] -----------                         \--*  LCL_VAR   byref  V03 tmp1         u:1 $140

***** BB01 [0000]
STMT00012
N004 (  5,  5) [000050] -----------                         *  JTRUE     void   $VN.Void
N003 (  3,  3) [000047] J------N---                         \--*  NE        int    $1c0
N001 (  1,  1) [000039] -----------                            +--*  LCL_VAR   byref  V03 tmp1         u:1 $140
N002 (  1,  1) [000046] -----------                            \--*  CNS_INT   byref  0 

[dotnet-policy-service]

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

[EgorBo]

PTAL @AndyAyersMS @jakobbotsch @dotnet/jit-contrib
Diffs. A couple of regressions where this assertion was useful, but in most cases "byref != null" was better.

[AndyAyersMS]

Does it make sense to assert non-null for both the byref value and the base value? I suppose the issue is on the checking side we're not doing the same add destructuring (which likely is somewhat reasonable, we probably do a lot more checking/searching then generating).

Looking at the removed code I am surprised it has to check for chains of adds. Does that mean VN is producing VN func trees of adds -- I would expect it ought to be folding up all the constant offsets into one? That would make it simpler to find the byref base as it should be right there (and given canonical commutation, always be say arg0.).

[EgorBo]

Does it make sense to assert non-null for both the byref value and the base value?

@AndyAyersMS My initial impl did exactly that but it's extra assertions and the benifts were small, Do you mind if I try again (with help of VN this time) to add assertions for base separately? So we can see actual diffs + TP impact

[AndyAyersMS]

Does it make sense to assert non-null for both the byref value and the base value?

@AndyAyersMS My initial impl did exactly that but it's extra assertions and the benifts were small, Do you mind if I try again (with help of VN this time) to add assertions for base separately? So we can see actual diffs + TP impact

Yeah go ahead and give it a try.

[EgorBo]

Does it make sense to assert non-null for both the byref value and the base value?

@AndyAyersMS My initial impl did exactly that but it's extra assertions and the benifts were small, Do you mind if I try again (with help of VN this time) to add assertions for base separately? So we can see actual diffs + TP impact

Yeah go ahead and give it a try.

Waiting for an approval then 🙂

[AndyAyersMS]

Ah, you mean in a follow-up PR... Sure

[LoopedBard3]

Potential regressions:

• Linux x64: [Perf] Linux/x64: 1 Regression on 5/8/2024 11:30:29 AM perf-autofiling-issues#34240
  Potential improvements:
• Linux x64: [Perf] Windows/x64: 1 Improvement on 5/8/2024 11:30:29 AM perf-autofiling-issues#34328