Fix Valuenum:EvalFuncForConstantArgs

[sandreenko]

For such tree:

N009 (  1,  1) [008525] ------------              |        /--*  CNS_INT   long   12 field offset Fseq[_firstChar]
N010 (  5,  4) [008526] ------------              |     /--*  ADD       byref
N008 (  3,  2) [008524] ------------              |     |  \--*  LCL_VAR   ref    V43 tmp35        u:4 (last use)

where LCL_VAR ref V43 is known to be a constant RyuJit calculated Vn as Null:
N010 [008526] ADD => $VN.Null
and merged it with other expressions with Null VN. Because constants were different in each case it caused bugs:

N003 (  1,  1) [008534] ------------              |  |  /--*  CNS_INT   long   8 field offset Fseq[_stringLength]
N004 (  4, 11) [008535] -------N----              |  \--*  ADD       byref
N002 (  3, 10) [008537] ------------              |     \--*  NOP       ref
N001 (  3, 10) [008536] ------------              |        \--*  CNS_INT(h) ref    0x421150 [ICON_STR_HDL]
N004 [008535]   ADD       => $VN.Null

VN [008535] == VN [008526] == Null, so all these constants were replaced with the first tree that had VN == Null:

VN based copy assertion for [004523] V16 @00000000 by [008560] V24 @00000000.
VN based copy assertion for [008648] V41 @00000000 by [008560] V24 @00000000
etc.

The original code was added in CS 946058 with other assert fixes and looks like we were lucky not to hit this issue for several years because CoreCLR doesn't produce constants with 'TYP_BYREF' (I think).

// We don't want to fold expressions that produce TYP_BYREF

I do not see any reasons why we should not fold them, but right now this code:
https://github.com/dotnet/coreclr/blob/master/src/jit/valuenum.cpp#L1813-L1817

doesn't not expect LONG add that produces BYREF and because our VN is very fragile we do not want to change it before the release, do we want to have an issue to fix that later?

The change will fix (some text to prevent closing this issue right after the merge) dotnet/corert#5661 after RyuJit version update.

[sandreenko]

PTAL @briansull , @dotnet/jit-contrib .
cc @MichalStrehovsky

[CarolEidt]

I am not intimately familiar with value numbering, but it seems surprising to me that we would effectively be considering a null VN to be a valid VN. It doesn't seem fundamentally that we couldn't fold TYP_BYREF constants, though I'm not sure when that would be useful - so it seems like this is a workaround for the problem that we seem to consider a null VN to be valid.
I would expect zero diffs for this - did you run them?

[briansull]

While this fix is fine, we should also fix the problem where the optimization was performed because both VN were 0. After we find the matching value number we shoudl assert that the value isn't 0 and then if it is we should bail out and not perform the optimization (so the retail builds are also correct)

[sandreenko]

but it seems surprising to me that we would effectively be considering a null VN to be a valid VN.

While this fix is fine, we should also fix the problem where the optimization was performed because both VN were 0.

Null is valid VN, look at ValueNum ValueNumStore::VNZeroForType(var_types typ):

coreclr/src/jit/valuenum.cpp

Lines 917 to 930 in 7ae64f0

	ValueNum ValueNumStore::VNZeroForType(var_types typ)
	{
	switch (typ)
	{
	case TYP_BOOL:
	case TYP_BYTE:
	case TYP_UBYTE:
	case TYP_SHORT:
	case TYP_USHORT:
	case TYP_INT:
	case TYP_UINT:
	return VNForIntCon(0);
	case TYP_LONG:
	case TYP_ULONG:

We have ValueNumStore::NoVN(UINT32_MAX) for invalid values.

[sandreenko]

I would expect zero diffs for this - did you run them?

Yes, no diffs for System.Private.CoreLib (x64).

[CarolEidt]

Null is valid VN

Ah, so if Null is the VN for a zero constant value, why are these nodes getting the Null VN?

[sandreenko]

Ah, so if Null is the VN for a zero constant value, why are these nodes getting the Null VN?

Because EvalFuncForConstantArgs returns ValueNum and return false; was casted to ValueNum(that is int), so this function returned 0(null) for all byrefs with constant arguments.

[briansull]

Looks Good

[briansull]

Thanks for clarifying that a null VN is currently a valid VN.
Perhaphs we should consider reserving the zero value so that it is invalid:
Adding to this enum would do the trick:

enum SpecialRefConsts
{
    SRC_ZeroIsIllegal,    // Reserved the value of zero, so that it is not a legal ValueNumber
    SRC_Null,

[briansull]

It looks like s_specialRefConsts needs some work in order to do this correctly