fgValueNumberIntrinsic() assert 'unreached' in corefx System.Runtime.Extensions.Tests

[BruceForstall]

Ubuntu x64 checked corefx_baseline test job:

Assert failure(PID 14495 [0x0000389f], Thread: 14520 [0x38b8]): Assertion failed 'unreached' in 'System.Tests.MathTests:Cbrt(double,double,double)' (IL size 14)
17:23:06   
17:23:06       File: /mnt/j/workspace/dotnet_coreclr/master/jitstress/x64_checked_ubuntu_corefx_baseline_prtest/src/jit/valuenum.cpp Line: 7235
17:23:06       Image: /mnt/j/workspace/dotnet_coreclr/master/jitstress/x64_checked_ubuntu_corefx_baseline_prtest/_/fx/bin/testhost/netcoreapp-Linux-Release-x64/dotnet
17:23:06   
...
17:23:10   /mnt/j/workspace/dotnet_coreclr/master/jitstress/x64_checked_ubuntu_corefx_baseline_prtest/_/fx/src/System.Runtime.Extensions/tests

https://ci.dot.net/job/dotnet_coreclr/job/master/job/jitstress/job/x64_checked_ubuntu_corefx_baseline_prtest/198

[BruceForstall]

@briansull @dotnet/jit-contrib

[briansull]

Assertion failed: /src/jit/valuenum.cpp Line: 7235

Missing case for new Instrinsic:

Called whenever we have a GT_INTRINSIC:

        else if (tree->OperGet() == GT_INTRINSIC)
        {
            fgValueNumberIntrinsic(tree);
        }
        else

void Compiler::fgValueNumberIntrinsic(GenTree* tree)
{
    assert(tree->OperGet() == GT_INTRINSIC);
    GenTreeIntrinsic* intrinsic = tree->AsIntrinsic();
    ValueNumPair      arg0VNP, arg1VNP;
    ValueNumPair      arg0VNPx = ValueNumStore::VNPForEmptyExcSet();
    ValueNumPair      arg1VNPx = ValueNumStore::VNPForEmptyExcSet();

    vnStore->VNPUnpackExc(intrinsic->gtOp.gtOp1->gtVNPair, &arg0VNP, &arg0VNPx);

    if (intrinsic->gtOp.gtOp2 != nullptr)
    {
        vnStore->VNPUnpackExc(intrinsic->gtOp.gtOp2->gtVNPair, &arg1VNP, &arg1VNPx);
    }

    switch (intrinsic->gtIntrinsicId)
    {
        case CORINFO_INTRINSIC_Sin:
        case CORINFO_INTRINSIC_Sqrt:
        case CORINFO_INTRINSIC_Abs:
        case CORINFO_INTRINSIC_Cos:
        case CORINFO_INTRINSIC_Round:
        case CORINFO_INTRINSIC_Cosh:
        case CORINFO_INTRINSIC_Sinh:
        case CORINFO_INTRINSIC_Tan:
        case CORINFO_INTRINSIC_Tanh:
        case CORINFO_INTRINSIC_Asin:
        case CORINFO_INTRINSIC_Acos:
        case CORINFO_INTRINSIC_Atan:
        case CORINFO_INTRINSIC_Atan2:
        case CORINFO_INTRINSIC_Log10:
        case CORINFO_INTRINSIC_Pow:
        case CORINFO_INTRINSIC_Exp:
        case CORINFO_INTRINSIC_Ceiling:
        case CORINFO_INTRINSIC_Floor:

            // GT_INTRINSIC is a currently a subtype of binary operators. But most of
            // the math intrinsics are actually unary operations.

            if (intrinsic->gtOp.gtOp2 == nullptr)
            {
                intrinsic->gtVNPair =
                    vnStore->VNPWithExc(vnStore->EvalMathFuncUnary(tree->TypeGet(), intrinsic->gtIntrinsicId, arg0VNP),
                                        arg0VNPx);
            }
            else
            {
                ValueNumPair newVNP =
                    vnStore->EvalMathFuncBinary(tree->TypeGet(), intrinsic->gtIntrinsicId, arg0VNP, arg1VNP);
                ValueNumPair excSet = vnStore->VNPExcSetUnion(arg0VNPx, arg1VNPx);
                intrinsic->gtVNPair = vnStore->VNPWithExc(newVNP, excSet);
            }

            break;

        case CORINFO_INTRINSIC_Object_GetType:
            intrinsic->gtVNPair =
                vnStore->VNPWithExc(vnStore->VNPairForFunc(intrinsic->TypeGet(), VNF_ObjGetType, arg0VNP), arg0VNPx);
            break;

        default:
            unreached();
    }
}

[BruceForstall]

fyi, also fails on Windows:

https://ci.dot.net/job/dotnet_coreclr/job/master/job/jitstress/job/x64_checked_windows_nt_corefx_baseline_prtest/249/consoleText

[briansull]

Suggested code to fix this issue:
Update fgValueNumverInstrinsic to call IsMathIntrinsic prior to using the switch stmt.

void Compiler::fgValueNumberIntrinsic(GenTree* tree)
{
    assert(tree->OperGet() == GT_INTRINSIC);
    GenTreeIntrinsic* intrinsic = tree->AsIntrinsic();
    ValueNumPair      arg0VNP, arg1VNP;
    ValueNumPair      arg0VNPx = ValueNumStore::VNPForEmptyExcSet();
    ValueNumPair      arg1VNPx = ValueNumStore::VNPForEmptyExcSet();

    vnStore->VNPUnpackExc(intrinsic->gtOp.gtOp1->gtVNPair, &arg0VNP, &arg0VNPx);

    if (intrinsic->gtOp.gtOp2 != nullptr)
    {
        vnStore->VNPUnpackExc(intrinsic->gtOp.gtOp2->gtVNPair, &arg1VNP, &arg1VNPx);
    }
    
    if (IsMathIntrinsic(intrinsic->gtIntrinsicId))
    {
        // GT_INTRINSIC is a currently a subtype of binary operators. 
        // But most of the math intrinsics are actually unary operations.
        
        if (intrinsic->gtOp.gtOp2 == nullptr)
        {
            intrinsic->gtVNPair =
                vnStore->VNPWithExc(vnStore->EvalMathFuncUnary(tree->TypeGet(), intrinsic->gtIntrinsicId, arg0VNP),
                                    arg0VNPx);
        }
        else
        {
            ValueNumPair newVNP =
                vnStore->EvalMathFuncBinary(tree->TypeGet(), intrinsic->gtIntrinsicId, arg0VNP, arg1VNP);
            ValueNumPair excSet = vnStore->VNPExcSetUnion(arg0VNPx, arg1VNPx);
            intrinsic->gtVNPair = vnStore->VNPWithExc(newVNP, excSet);
        }
    }
    else // Handle the non-"Math Instrinsic" case(s)
    {
        switch (intrinsic->gtIntrinsicId)
        {
        case CORINFO_INTRINSIC_Object_GetType:
            intrinsic->gtVNPair =
                vnStore->VNPWithExc(vnStore->VNPairForFunc(intrinsic->TypeGet(), VNF_ObjGetType, arg0VNP), arg0VNPx);
            break;
            
        default:
            unreached();
        }
    }
}

[briansull]

@tannergooding I am assigning this issue to you
You can check in the fix that I suggested.
If CoreFX testing used a checked JIT you would have seen this failure when adding the new tests.

As it is now (using a retail JIT) it probably thows an noway_assert and then we fall back to running the test with optimization disabled and that means that we ignore the Intrinsic optimizations.

[tannergooding]

Thanks. I should have a PR up shortly.

[tannergooding]

PR is up here: dotnet/coreclr#16288, still validating that everything passes locally. Sorry for the trouble.