[RyuJIT] Lowering is inconsistent in checking safety of RegOptional

[CarolEidt]

In a number of places, Lowering checks IsSafeToContainMem() before setting an operand as contained, then if it fails to make it contained it marks it as regOptional. However, if it is unsafe for the node to be contained, it is presumably also unsafe to mark it regOptional, as it will be loaded at the point of use in the event that it doesn't get a register.

category:correctness
theme:lower
skill-level:expert
cost:small

[CarolEidt]

@pgavlin @BruceForstall - is there something I may be missing here?
cc @dotnet/jit-contrib

[CarolEidt]

Note that I haven't found an actual bug, but it seems incorrect on examination.

[pgavlin]

However, if it is unsafe for the node to be contained, it is presumably also unsafe to mark it regOptional, as it will be loaded at the point of use in the event that it doesn't get a register.

Let's break this down by node/interval kind:

• For reg-candidate lclVars, each use node translates to a use position at the point at which the using node executes. In order to ensure that this is safe, the IR is constructed s.t. no redefinitions of the lclVar may appear between a use node for a lclVar V0 and the using node. Therefore, if the operand is marked regOptional and does not get a register, neither the point of use nor the value stored in the lclVar's stack slot change and the transformation is safe.
• For nodes that define SDSU temps, each node translates to an interval with exactly one def position and exactly one use position. If I understand correctly, a def position may not be reg-optional. If this is the case, then each of these intervals must be allocated a register at the def. The RA can then avoid allocating a register to the use if the def is spilled, in which case the value is treated as a memory operand that references the spill slot. In this case, both def and use still occur at the same locations and the transformation is safe.

Given the above--namely, that in the reg-candidate lclVar case it is always safe to move a use forwards to the user and in the SDSU temp case the defs/uses are not actually moving--I do not think that it is necessary for us to do the same sort of legality checks that we do for contained operands when marking operands as regOptional.

This is all moot, of course, if I've misunderstood the semantics of regOptional operands :)

[CarolEidt]

For nodes that define SDSU temps, each node translates to an interval with exactly one def position and exactly one use position.

The trickiness comes about for non-reg-candidate lclVars. These mostly behave as SDSU temps, but if they are marked regOptional and do not get a register, they will be loaded from their home location at the point of use.

[pgavlin]

The trickiness comes about for non-reg-candidate lclVars. These mostly behave as SDSU temps, but if they are marked regOptional and do not get a register, they will be loaded from their home location at the point of use.

Ah, sure, since rather than using a spill slot we elide the spill (I assume) and reload/use from the lclVar's stack slot. That is definitely not safe without additional checks.

[pgavlin]

To be more precise, that transform should be unsafe only for aliased (e.g. address-exposed) lclVars. It is safe for lclVars that are not aliased even if they are not register candidates.

[CarolEidt]

With thanks to @jakobbotsch and @mikedn it seems that we have a repro case for this. Although the issue in which this came up is linked above, I'm duplicating it here:

// Debug: Outputs 0
// Release: Outputs 1
public class Program
{
    public static void Main()
    {
        long vr4 = 0;
        long vr5 = vr4 ^ System.Threading.Interlocked.Exchange(ref vr4, 1);
        System.Console.WriteLine(vr5);
    }
}

The code generated is:

G_M5092_IG01:
       4883EC28             sub      rsp, 40
       33C0                 xor      rax, rax
       4889442420           mov      qword ptr [rsp+20H], rax

G_M5092_IG02:
       33C9                 xor      rcx, rcx
       48894C2420           mov      qword ptr [rsp+20H], rcx
       488D4C2420           lea      rcx, bword ptr [rsp+20H]
       B801000000           mov      eax, 1
       488701               xchg     qword ptr [rcx], rax
       488BC8               mov      rcx, rax
       48334C2420           xor      rcx, qword ptr [rsp+20H]
       E82CFCFFFF           call     System.Console:WriteLine(long)
       90                   nop

G_M5092_IG03:
       4883C428             add      rsp, 40
       C3                   ret

as @mikedn observes:

It looks like in this case the second operand of xor was incorrectly marked as reg optional which basically caused the load from [rsp+20h] to move past xchg. If I disable that reg optional code the following code is generated:

       33C9                 xor      rcx, rcx
       48894C2420           mov      qword ptr [rsp+20H], rcx
       488B4C2420           mov      rcx, qword ptr [rsp+20H]
       488D442420           lea      rax, bword ptr [rsp+20H]
       BA01000000           mov      edx, 1
       488710               xchg     qword ptr [rax], rdx
       4833CA               xor      rcx, rdx
       E804FCFFFF           call     System.Console:WriteLine(long)

[CarolEidt]

@erozenfeld - when I opened this issue I didn't actually have an example that exhibited it. Now that we have one, I would say this raises the priority, not to mention making it easier to validate that it's fixed!

[erozenfeld]

Thank you @CarolEidt . I'll work on a fix for this soon.

[mikedn]

On a side note watch out that ContainCheckBinary might have another bug - it calls IsSafeToContainMem only for op1, not for op2. I noticed that in the past but I could not construct an example where this would cause problems. It may be that it can't happen for op2 to be unsafe to contain due to how IR is constructed but it's certainly not obvious that op2 is safe.

[CarolEidt]

it calls IsSafeToContainMem only for op1, not for op2

It may have be done on the assumption that, in the normal case, op1 will be evaluated first, so that op2's evaluation will occur just prior to the instruction in question. However, with LIR there's no guarantee of that, and we no longer have the reverse flag to know which is evaluated first. So I think that we need to be checking both.