@AndyAyersMS Do you happen to know why would assertionprop's optPrepareTreeForReplacement use GTF_PERSISTENT_SIDE_EFFECTS_IN_CSE to extract the replaced tree's side effects? This function is used only during VN based assertion propagation so it's not clear why it cares about CSE.

No, I don't know why -- maybe @briansull or @erozenfeld can chime in.

Th CSE phase can eliminate some SIDE_EFFECTS because it can guarantee that the SIDE_EFFECT is done once at the def locations and can be skipped at the use locations. Some but not all, side effect are allowed to be eliminated in this way.

The definition of GTF_PERSISTENT_SIDE_EFFECTS_IN_CSE has more details:

// The extra flag GTF_IS_IN_CSE is used to tell the consumer of these flags
// that we are calling in the context of performing a CSE, thus we
// should allow the run-once side effects of running a class constructor.
//
// The only requirement of this flag is that it not overlap any of the
// side-effect flags. The actual bit used is otherwise arbitrary.
#define GTF_IS_IN_CSE GTF_BOOLEAN
#define GTF_PERSISTENT_SIDE_EFFECTS_IN_CSE (GTF_ASG | GTF_CALL | GTF_IS_IN_CSE)

Th CSE phase can eliminate some SIDE_EFFECTS because it can guarantee that the SIDE_EFFECT is done once at the def locations and can be skipped at the use locations. Some but not all, side effect are allowed to be eliminated in this way.

Yes, the CSE is somewhat special in its handling of side effects. But this is about assertion propagation so it's not clear why GTF_IS_IN_CSE is used here and it appears to contradict the comment that says "we are calling in the context of performing a CSE".

For clarity - this is intended to be a fix for #18232

And the same optPrepareTreeForReplacement is also responsible for dropping some volatile reads, probably because it doesn't specify GTF_ORDER_SIDEEFF.

GTF_PERSISTENT_SIDE_EFFECTS_IN_CSE allows gtExtractSideEffList to ignore .cctor calls. This is valid only if the caller somehow knows that such calls are redundant. CSE does have this knowledge when removing use trees because it is expected that the def tree will also include the necessary .cctor call. No other gtExtractSideEffList caller seem to have this knowledge (including assertion propagation) so that's probably why the flag refers specifically to CSE rather than saying something like "IGNORE_CCTORS".

Now, the main use of optPrepareTreeForReplacement is to drop trees that VN says that are constants. A .cctor call normally carries an exception side effect so one might expect that the containing tree will never end up being a VN constant and thus using GTF_PERSISTENT_SIDE_EFFECTS_IN_CSE makes no difference. But VN doesn't consistently propagate exceptions so it's actually possible to end up with a constant VN on a tree having exception side effects and .cctor calls. So optPrepareTreeForReplacement must not use GTF_PERSISTENT_SIDE_EFFECTS_IN_CSE and at the same time it needs to include GTF_EXCEPT, otherwise we'll end up dropping exceptions.

Somewhat similarly, it is possible to end up with a constant VN even if the tree contains volatile reads, at least in corner cases such as x & 0.

It seems to me that optPrepareTreeForReplacement really needs to use GTF_ALL_EFFECT because nothing guarantees that the tree to be replaced contains only persistent side effects.

It seems to me that optPrepareTreeForReplacement really needs to use GTF_ALL_EFFECT because nothing guarantees that the tree to be replaced contains only persistent side effects.

Which does not actually help with the case of dropped volatile reads because those aren't side effects, at least not according to gtNodeHasSideEffects. So in the end it just needs to be GTF_SIDE_EFFECT. Oh well, side effects are a horror story...

The change to GenTreeVisitor causes 2 similar differences in System.Runtime.Serialization.Formatters:

        mov      r9, r12
-       mov      r10d, dword ptr [r9+8]
        lea      r10d, [rax+rcx]
+       mov      r11d, dword ptr [r9+8]
        movsxd   r10, r10d

This is the result of a range check being eliminated and old gtExtractSideEffList failing to preserve the original side effect ordering.

The change to use GTF_SIDE_EFFECT generates one diff:

        xor      ebp, ebp
        mov      r13d, dword ptr [rsp+74H]
        test     r13d, r13d
        jle      G_M19407_IG19
-       mov      rax, gword ptr [rsp+60H]
-       cmp      dword ptr [rax+8], r13d
+       test     r13d, r13d
        setge    cl
        movzx    rcx, cl
-       test     r13d, r13d
+       mov      eax, dword ptr [r15+8]
+       and      ecx, 1
+       mov      rax, gword ptr [rsp+60H]
+       cmp      dword ptr [rax+8], r13d
        setge    dl
        movzx    rdx, dl
-       and      edx, 1
        and      ecx, edx
        mov      rdx, gword ptr [rsp+68H]
        cmp      dword ptr [rdx+8], r13d

This is somewhere in System.Data.Common - https://github.com/dotnet/corefx/blob/ef03e596a20c87c2793fa31a39cc2bbbb03831dd/src/System.Data.Common/src/System/Data/XMLSchema.cs#L489

The actual change is an extra mov eax, dword ptr [r15+8]. The rest of changes are the result of re-ordering.

The for loop got cloned and then one of conditions got evaluated to true by VN. The lack of GTF_EXCEPT resulted in a GT_ARR_LENGTH node being dropped.

Now, since VN managed to evaluated the condition it probably means that an exception can't ever be thrown, the array reference can't be null. But getting this right requires either VN to sort out its exception propagation (presumably related to #8648) or a way to figure out that GT_ARR_LENGTH can't throw in this case (e.g fgAddrCouldBeNull could perhaps use ValueNumStore::IsKnownNonNull).

I hadn't seen that this wasn't WIP anymore.
This fixes a whole bunch of cases:
jakobbotsch/Fuzzlyn@4b1fa07

There are some that still throw, such as this one:

// Generated by Fuzzlyn on 2018-06-26 16:32:41
// Seed: 3807292053929899503
// Reduced from 119.1 KiB to 0.2 KiB
// Debug: Runs successfully
// Release: Throws 'System.NullReferenceException'
public class Program
{
    static ushort[, ] s_1 = new ushort[, ]{{0}};
    public static void Main()
    {
        bool vr5 = 0 == ((0 % ((0 & s_1[0, 0]) | 1)) * s_1[0, 0]);
    }
}

Is this a separate issue?

There are some that still throw, such as this one:

Uh oh, it's a different issue but in the same area. This PR basically fixes optPrepareTreeForReplacement (by fixing gtExtractSideEffList) but then it turns out that there are cases where optPrepareTreeForReplacement is misused.

It's really baffling, optVNConstantPropOnTree calls optPrepareTreeForReplacement(tree, tree) and then changes tree to a constant. In this case

• tree is MOD so itself ends up as a side effect
• tree is changed to a constant so the entire side effect subtree simply vanishes
• newTree is a COMMA where both op1 and op2 point to the same constant node

I'm surprised that this doesn't blow up more often and more spectacularly!

I have submitted #18648 for that. It's interesting that there are two similar issues in this area.
It required changes in a couple of examples too: jakobbotsch/Fuzzlyn@b5113d2

The above discussed issue is a consequence of changing optPrepareTreeForReplacement to use GTF_SIDE_EFFECT instead of GTF_PERSISTENT_SIDE_EFFECTS. Nodes that can end up evaluated to constants can't have persistent side effects (they're not assignments/calls) but they may have exception side effects (GTF_EXCEPT) even if they never throw (it can't evaluate to a constant and throw at the same time).

GTF_SIDE_EFFECT is needed because the children of the constant node may throw exceptions (e.g. s_1[0, 0] can throw IndexOutOfRangeException).

The proper solution is likely to pass ignoreRoot to gtExtractSideEffList so that the constant node is never extracted as a side effect.

@dotnet/jit-contrib This is good for review, although I'm not very happy with the newTree == oldTree special handling in optPrepareTreeForReplacement. But I can't think of a better solution, other than simply creating new constant nodes instead of attempting to change existing nodes.

There are some examples left which could be related:

// Generated by Fuzzlyn on 2018-06-26 03:15:42
// Seed: 6043644693530160524
// Reduced from 196.7 KiB to 0.3 KiB
// Debug: Runs successfully
// Release: Throws 'System.IndexOutOfRangeException'
public class Program
{
    public static void Main()
    {
        var vr16 = new ulong[]{18446744073709551614UL};
        M5(vr16);
    }

    static byte M5(ulong[] arg0)
    {
        int var0 = (ushort)((0 % arg0[0]) | (byte)(-32768 * (int)(0 & arg0[0])));
        System.Console.WriteLine(var0);
        return 0;
    }
}

A lot of examples still throw, but a lot of them are probably #18770.

There are some examples left which could be related:

Hmm, your example works fine with both the master branch and my branch. Am I missing something?

The comparison is done with stack garbage, so it depends on whether the stack slot is zero or not:

; Assembly listing for method Program:M5(ref):ubyte
; Emitting BLENDED_CODE for X64 CPU with AVX
; optimized code
; rsp based frame
; partially interruptible
; Final local variable assignments
;
;  V00 arg0         [V00,T00] ( 23, 23   )     ref  ->  rcx         class-hnd
;  V01 OutArgs      [V01    ] (  1,  1   )  lclBlk (32) [rsp+0x00]
;  V02 cse0         [V02,T01] ( 12, 12   )    long  ->  rdx
;  V03 cse1         [V03,T02] (  8,  8   )     int  ->  [rsp+0x24]
;
; Lcl frame size = 40

G_M23191_IG01:
       4883EC28             sub      rsp, 40

G_M23191_IG02:
       837C242400           cmp      dword ptr [rsp+24H], 0 ; that was quick..?
       761D                 jbe      SHORT G_M23191_IG04
       8B4110               mov      eax, dword ptr [rcx+16]
       33C0                 xor      rax, rax
       8B5108               mov      edx, dword ptr [rcx+8]
       33D2                 xor      rdx, rdx
       48F77110             div      rdx:rax, qword ptr [rcx+16]
       0FB7CA               movzx    rcx, dx
       E8DFFBFFFF           call     System.Console:WriteLine(int)
       33C0                 xor      eax, eax

G_M23191_IG03:
       4883C428             add      rsp, 40
       C3                   ret

G_M23191_IG04:
       E8F3D8F25E           call     CORINFO_HELP_RNGCHKFAIL
       CC                   int3

; Total bytes of code 46, prolog size 4 for method Program:M5(ref):ubyte
; ============================================================

EDIT: That looks very weird. I'm guessing a different issue?

The comparison is done with stack garbage, so it depends on whether the stack slot is zero or not:

Right, missed that. I'll take a look, right now what I can tell for sure is that it doesn't appear to be related to this fix. The same code is generated before and after this change.

P.S.

Yep, this just another case of use-def re-ordering. It may be worth pointing out that this re-ordering also results in the elimination of the def as a dead store. That's why there's only a single reference to [rsp+24h] in the entire code.