RyuJIT sometimes drops volatile reads

[mikedn]

Morph issue:

static volatile int x;

[MethodImpl(MethodImplOptions.NoInlining)]
static int Test(int a) => x & 0;

generates

G_M55886_IG01:
G_M55886_IG02:
       33C0                 xor      eax, eax
G_M55886_IG03:
       C3                   ret

Similar issue but involving VN/assertion propagation:

static int Test(int a)
{
    int k = a > 42 ? 0 : 0;
    return x & k;
}

generates the same code as above.
In this case VN assigned x & k the VN of constant 0 and then assertion propagation drops the x load because it only considers persistent side effects.

Found while working on dotnet/coreclr#18257. Initially I thought that we can simple make assertion propagation use GTF_ALL_EFFECT only to discover that it doesn't work because gtNodeHasSideEffects doesn't pay attention to side effects other than GTF_SIDE_EFFECT. So who's paying attention to GTF_ORDER_SIDEEFF and GTF_GLOB_REF?!

category:correctness
theme:volatile
skill-level:expert
cost:large

[AndyAyersMS]

Lost volatile reads are hard to detect via testing. Read side effects are pretty rare.

In past compilers we'd introduce extra hurdles to create or delete volatile operands and try to account for them all somehow. To first order, the number of volatile nodes should remain constant across each phase (there are of course exceptions).

Given how lax the jit is with node deletion I wonder how well that could work here. Perhaps we'd first need to start some behind the scenes accounting to track leaked nodes (unused and not explicitly deleted). Might be worth doing anyways as we could then insist (in checked builds anyways) that the jit explicitly clear out/delete unused nodes, and leak detection has other merits, eg tracking down phases that create unused IR for no good reason.

Once we have that and we've stamped out all leaks, we can then make it invalid to delete volatile operands without using special methods.

[mikedn]

Might be worth doing anyways as we could then insist (in checked builds anyways) that the jit explicitly clear out/delete unused nodes, and leak detection has other merits, eg tracking down phases that create unused IR for no good reason.

Perhaps that can be done in release builds as well - deleted nodes would end up in a list that can also be used as a node pool to avoid allocating new ones. That may be better than some contortions the JIT currently does in an attempt to reuse nodes (I'm looking at you optVNConstantPropOnTree).

[AndyAyersMS]

That's an interesting idea. Wonder how effective it would be....

[mikedn]

It's interesting that GT_MEMORYBARRIER ends up with a GTF_ASG flag. Using the same reasoning volatile indirs should get GTF_ASG too. Basically GTF_ASG has stopped being just "sub-expression contains an assignment", today it's meaning is more like "some observable and persistent side effect".

[mikedn]

Given how lax the jit is with node deletion I wonder how well that could work here.

Random idea of the day: perhaps we can make it lax and safe at the same time. Keep only side effects in linear order and leave the rest out.

Then changing a node to a constant (like assertion propagation does) requires no extra work - whatever side effects the constant tree contains remain in the linear order and the rest simply vanish because they're no referenced by anything (this assumes we get rid of incremental ref count updates, otherwise "the rest" can't just vanish).

Side effects would be removed from the linear order only by code that know what it is doing and not by accident. We'd probably get rid of gtExtractSideEffList and possibly gtTreeHasSideEffects/gtUpdateSideEffects as well.

Granted, not having every node in linear order means using tree walkers more and those are less efficient. But all the work required maintain linear order through trivial expression transforms and extract/update side effects has its costs, especially that it tends to be done repeatedly.

[AndyAyersMS]

Don't see us fixing this for 3.0 -- so moving to future.

[teo-tsirpanis]

If I read the memory model correctly, reads have no side-effects and therefore are allowed to be dropped, regardless of their volatile status. Should we close this as by design?