JIT: Fix icon and vn mismatch in an assertion

[EgorBo]

No description provided.

[EgorBo]

PTAL @jakobbotsch @AndyAyersMS @dotnet/jit-contrib
Simple change - there was an unnecessary cast for icon in LCLVAR assertion which led to mismatch in op2.vn and op2.u1.icon. Also, for some reason it was disabled for arm32

Diffs - improvements on arm32

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

[jakobbotsch]

What was the case that asserted? The existing code looks more right to me if the assertion is being created from GT_STORE_LCL_VAR for a normalize-on-store local -- that comes with an implicit normalization.

[EgorBo]

What was the case that asserted? The existing code looks more right to me if the assertion is being created from GT_STORE_LCL_VAR for a normalize-on-store local -- that comes with an implicit normalization.

the assertion is LCL (not)equals icon assertion users have everything they need to perform any normalization if they need? the assert is the one I put recently - comparing op2.vn and op2.u1.icon - they basically become two different constants

[EgorBo]

int cnstLimit = (int)curAssertion->op2.u1.iconVal;
assert(cnstLimit == comp->vnStore->CoercedConstantValue<int>(curAssertion->op2.vn));

with the code in Main, it fails with 255 being compared against -1
while the original tree that created this assertion was:

JTRUE
  lcl(ubyte) != -1

[jakobbotsch]

Is that local normalize-on-store or normalize-on-load? Comparison is weird anyway, seems like it would always be true...

[EgorBo]

@jakobbotsch it's from Fuzzlyn, I just didn't add the repro because it doesn't seem to be a real issue, I just wanted to fix Fuzzlyn issues, let me know if you want to me to add it

public static Vector<uint> s_4;
public static ulong s_12 = 1;
public static void Main()
{
    byte vr5 = default(byte);
    var vr6 = (double)0;
    var vr7 = Vector128.CreateScalar(vr6);
    var vr8 = Vector128.CreateScalar(2876.118710215498d);
    var vr9 = Vector128.Create<double>(0);
    var vr10 = Sse2.UnpackHigh(vr8, vr9);
    if (Sse2.CompareScalarUnorderedLessThan(vr7, vr10))
    {
        vr5 ^= 1;
    }

    if (vr5 == -1) // <---- this one creates the assertion, the condition is always false
    {
        Vector<uint> vr11 = s_4;
    }
}

...
N005 (  4,  5) [000039] DA---+-----                         *  STORE_LCL_VAR int    V00 loc0         d:3 $VN.Void
N004 (  4,  5) [000038] -----+-----                         \--*  CAST      int <- ubyte <- int $44
N003 (  3,  3) [000037] -----+-----                            \--*  XOR       int    $44
N001 (  1,  1) [000035] -----+-----                               +--*  LCL_VAR   int    V00 loc0         u:2 (last use) $40
N002 (  1,  1) [000036] -----+-----                               \--*  CNS_INT   int    1 $44

------------ BB03 [0002] [04B..04F) -> BB05(0.5),BB04(0.5) (cond), preds={} succs={BB04,BB05}

***** BB03 [0002]
STMT00011 ( ??? ... ??? )
N004 (  0,  0) [000043] DA---------                         *  STORE_LCL_VAR ubyte  V00 loc0         d:4 $VN.Void
N003 (  0,  0) [000042] -----------                         \--*  PHI       ubyte  $280
N001 (  0,  0) [000045] ----------- pred BB02                  +--*  PHI_ARG   ubyte  V00 loc0         u:3 $44
N002 (  0,  0) [000044] ----------- pred BB01                  \--*  PHI_ARG   ubyte  V00 loc0         u:2 $40

***** BB03 [0002]
STMT00007 ( 0x04B[E-] ... 0x04D )
N004 (  5,  5) [000026] -----+-----                         *  JTRUE     void   $VN.Void
N003 (  3,  3) [000025] N----+-N-U-                         \--*  NE        int    $241
N001 (  1,  1) [000023] -----+-----                            +--*  LCL_VAR   int    V00 loc0         u:4 (last use) $280
N002 (  1,  1) [000024] -----+-----                            \--*  CNS_INT   int    -1 $41

[jakobbotsch]

Can we change it so that we retain the cast if the assertion was created because of a STORE_LCL_VAR to a normalize-on-store local? I would be worried removing the cast for that case would introduce bad codegen.

[EgorBo]

@jakobbotsch so just return the cast back and update vn will work?

[jakobbotsch]

The VN is correct for the equality comparison case (and the cast is wrong in that case). But if this path also gets hit from here:

runtime/src/coreclr/jit/assertionprop.cpp

Lines 2410 to 2415 in 3d27554

	case GT_STORE_LCL_VAR:
	// VN takes care of non local assertions for data flow.
	if (optLocalAssertionProp)
	{
	assertionInfo = optCreateAssertion(tree, tree->AsLclVar()->Data(), OAK_EQUAL);
	}

then it would probably result in potential bad codegen to remove the cast.

[jakobbotsch]

The cast was introduced to fix a silent bad codegen bug in #71336

[jakobbotsch]

I think you can change the check to:

if (varTypeIsSmall(lclVar) && op1->OperIs(GT_STORE_LCL_VAR)