There needs to be a proper "new, unique" value number for exception sets

[SingleAccretion]

Enable the debug checking and full fidelity exception sets for all calls, including user calls. This would entail optimizing the opaque exception to not consume memory by using a custom chunk kind.

category:implementation
theme:value-numbering
skill-level:intermediate
cost:small
impact:small

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

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

Issue Details

---

Reproduction, run on 32 bit:

using System.Runtime.CompilerServices;

Problem(0, 1, false, false, 0);

[MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)]
static bool Problem(long a, long b, bool c1, bool c2, int i)
{
    var zero = 0;
    c1 = ((a / b) * zero) + i == 0;
    c2 = ((b / a) * zero) + i == 0;

    return c2 & c1;
}

Expected result: DivideByZeroException.
Actual result: program runs successfully.

Cause: in the VN logic, HelperMultiplExc is assigned to helpers that can throw some unknown exception. That is too weak, as HelperMultiplExc is a singleton VNFunc (only ever has one numeric VN assigned to it). We need the equivalent of VNForExpr for exception sets to model "an unknown" exception. Using it we will also be able to model user calls "properly" (propagate the exception sets for them), making the exception sets fully precise.

Creating such exception VNs should be "zero-cost", i. e. consume no actual memory as they do not have useful input to store.

Author:	SingleAccretion
Assignees:	SingleAccretion
Labels:	bug, area-CodeGen-coreclr
Milestone:	-

[SingleAccretion]

It is notable that "just" giving a unique value number to helpers that today receive the HelperMultipleExc VN will not work (or, rather, it will work, but lead to huge regressions). For example, it will make all the "get static base" calls non-CSEable. Yet, today those can be silently dropped, so there is clearly a correctness issue to be solved, as, for example, the below program will not throw an exception, even as it must.

using System;
using System.Runtime.CompilerServices;

Console.WriteLine(Problem(0, false, false));

[MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.NoOptimization)]
private static bool Problem(int b, bool c1, bool c2)
{
    var zero = 0;
    c1 = ClassWithCctorNoThrow.StaticField * zero + b == 0;
    c2 = c1;

    if (ClassWithCctor.StaticField * zero + b == 0)
    {
        Console.WriteLine("No exception was thrown...");
        return false;
    }

    return c2;
}

static class ClassWithCctor
{
    public static int StaticField;
    static ClassWithCctor() => throw new Exception();
}

static class ClassWithCctorNoThrow
{
    public static int StaticField;
    static ClassWithCctorNoThrow() { }
}

[SingleAccretion]

The problem with this issue was that actually creating "new, unique" exception sets for calls lead to very large memory consumption regression; I don't think I'll have time to think of a way out of that for 7.0.

[jakobbotsch]

#101062 fixes the issue around JIT helpers. The problem of modelling exception sets for user calls remain.
@SingleAccretion Do you know of any correctness issues related to user call exception sets here? If not I think we can push this out of 9.0.

[SingleAccretion]

Do you know of any correctness issues related to user call exception sets here?

There are none. Yeah, no reason to keep it in 9.0 (the issues here were ancient anyway).

[jakobbotsch]

Sounds good. I'll give this back to you (or feel free to unassign yourself...)