[RyuJIT] Unsigned binary operation with overflow check

[mikedn]

Remember #7439? I ran into that again but this time it's a real bug.

The importer does this: https://github.com/dotnet/coreclr/blob/a6045809b3720833459c9247aeb4aafe281d7841/src/jit/importer.cpp#L9374-L9396

Note that it selects the "castType" based on the signedness of the operation but it does not set the GTF_UNSIGNED flag on the cast.

So if it doesn't set the flag how come we zero extend int32 in the case of add.ovf.un? That's because CodeGen::genIntToIntCast does something weird: https://github.com/dotnet/coreclr/blob/a6045809b3720833459c9247aeb4aafe281d7841/src/jit/codegenxarch.cpp#L6490-L6497

It selects INS_movsxd if the source is signed. And if the destination is signed! That doesn't make sense, the sign of the destination is relevant only for overflow checks, it should have no impact on the kind of widening performed.

I was curious what the ARM64 codegen does and of course, it doesn't consider the destination sign and emits sxtw:

B9802C09          ldrsw   x9, [x0,#44]
93407D29          sxtw    x9, x9
F9400C2A          ldr     x10, [x1,#24]
AB0A0129          adds    x9, x9, x10
54000062          bhs     G_M55886_IG04

For the same code the x64 codegen produces mov, not movsxd.

So, if we want to keep the existing x64 behavior (zero extend add.ovf.un operands) we need to:

• make the importer set GTF_UNSIGNED when needed (this will make ARM64 codegen generate the required code)
• change x64 codegen to ignore the destination type when selecting INS_movsxd

Though after seeing this I'm more inclined to consider the current behavior to be a JIT bug and that sign extension should be performed. So:

• change x64 codegen to ignore the destination type when selecting INS_movsxd

And either way we probably need to add tests for this. I only noticed this while looking at diffs in dotnet/coreclr#12676. All tests pass but the code isn't right.

[mikedn]

cc @CarolEidt @BruceForstall

[mikedn]

An example to better illustrate the issue:

byte* p = ...;
int offset1 = -1;
byte* p1 = checked(p + offset1);

-1 is treated as uint and the following code is generated

488B442420           mov      rax, bword ptr [rsp+20H]
BAFFFFFFFF           mov      edx, 0xFFFFFFFF
4803C2               add      rax, rdx
720C                 jb       SHORT G_M55886_IG04

4294967295 is added to the pointer and no OverflowException is thrown.
Let's produce -1 a bit differently:

int offset2 = b ? -1 : -1;
byte* p2 = checked(p + offset2);

Now -1 is treated as int (I haven't checked where, could be in VN's constant eval) and the following code is generated:

488B442420           mov      rax, bword ptr [rsp+20H]
4883C0FF             add      rax, -1
720C                 jb       SHORT G_M55886_IG04

Of course, this throws OverflowException.

Let's mix the 2:

int offset1 = -1;
int offset2 = b ? -1 : -1;
byte* p1 = checked(p + offset1);
byte* p2 = checked(p + offset2);

Not in both cases we add -1:

488B442420           mov      rax, bword ptr [rsp+20H]
4883C0FF             add      rax, -1
7217                 jb       SHORT G_M55886_IG04
488B442420           mov      rax, bword ptr [rsp+20H]
4883C0FF             add      rax, -1
720C                 jb       SHORT G_M55886_IG04

Haven't checked how this happens, perhaps VN ended up with the same VN for both casts.

Anyway, it's clear that RyuJIT's handling of add.ovf.un is broken.

[BruceForstall]

cc @dotnet/jit-contrib

[mikedn]

So the differences show in the above examples are the result of differences between Compiler::gtFoldExprConst and ValueNumStore::EvalCastForConstantArgs. The first does this:

https://github.com/dotnet/coreclr/blob/fa79179baa11923cba48e2cdd9c9bd723486a136/src/jit/gentree.cpp#L12602-L12619

The second does this:

https://github.com/dotnet/coreclr/blob/fa79179baa11923cba48e2cdd9c9bd723486a136/src/jit/valuenum.cpp#L2004-L2030

When casting to TYP_ULONG gtFoldExprConst disregards GTF_UNSIGNED and always zero extends. But EvalCastForConstantArgs does use srcIsUnsigned and that one is set based on GTF_UNSIGNED.

I don't know which one is correct but I consider EvalCastForConstantArgs's behavior to be saner. It doesn't make sense to me for GTF_UNSIGNED to be used or not depending on the destination type.

I suppose that along the way there was some confusion around cast representation in IR and in IL. In IL conv.i8 and conv.u8 do ignore the source type and extend based on the destination type. But that's natural in IL where the source type can never be unsigned.

When importing conv.i8 and conv.u8 the GTF_UNSIGNED flag is set as expected so this issue does not affect normal casts. We only get into trouble when the JIT automatically inserts a cast to TYP_ULONG and intentionally or accidentally doesn't set GTF_UNSIGNED.

[VSadov]

Just wonder - considering this issue, do we still want to fix dotnet/roslyn#18871 or we may be reconsidering?

[CarolEidt]

Just wonder - considering this issue, do we still want to fix dotnet/roslyn#18871 or we may be reconsidering?

I believe that this is a different case, because the cast is actually there in the IL, but not handled correctly by the JIT, whereas in the other case there was no cast.

[VSadov]

@CarolEidt - thanks!! I was not sure if the issues are related. The plan does not change then - at some point we will fix dotnet/roslyn#18871 to emit different IL as was discussed.

[mikedn]

FWIW I think that the JIT should be changed to sign extend add.ovf.un operand:

• int is far more common in the .NET world than uint. Requiring the C# compiler to insert a cast in the common case defeats the purpose of allowing add.ovf.un to have mixed type operands.
• add.ovf.un(p, -1) throws OverflowException on 32 bit. On 64 bit it does not throw when -1 is zero extended. That too seems to defeat the purpose of add.ovf.un with native int/int operands.
• It certainly doesn't look like RyuJIT x64 zero extends the operand by design.

Of course, there's the pesky issue of the legacy JIT64 also zero extending...

[mikedn]

I believe that this is a different case, because the cast is actually there in the IL, but not handled correctly by the JIT, whereas in the other case there was no cast.

@CarolEidt No, it's the same case. Casts that are present in IL are handled correctly, only some casts inserted by the JIT have this problem.

[VSadov]

FWIW, it seems sensible to always cast and stop relying on add.ovf.un support for mixed arguments altogether. That would be the most reliable way of fixing dotnet/roslyn#18871
If we have to force sign extending for int operands, the cast is needed in nearly 100% cases anyways.

[mikedn]

it seems sensible to always cast and stop relying on add.ovf.un support for mixed arguments altogether

Right, that's probably the only way to ensure that an assembly compiled by a new version of the C# compiler behaves the same on all runtimes.

[JosephTremoulet]

FWIW I think that the JIT should be changed to sign extend add.ovf.un operand

I'm inclined to agree, and I'll throw in another reason: I'd like to think that the ovf suffixes affect only whether an operation throws an overflow exception, not the numerical result of the operation. When we have a simple add of nint + int32, we sign-extend, right? So then I'd expect add.ovf.un to similarly sign-extend, and just check for overflow on the addition. Otherwise, on a 64-bit platform, for values that don't overflow regardless of extension type, like nullptr plus 0x80000000, changing opcode from add to add.ovf.un would change the result from 0xFFFFFFFF80000000 to 0x00000000800000000.

[mikedn]

When we have a simple add of nint + int32, we sign-extend, right?

Yes, we sign extend. And of course, there's no add.un to request a zero extend.

[mikedn]

Hint hint... I'm happy to make the necessary code changes provided that a decision is taken...

[CarolEidt]

Here's my take on this issue:

• Regarding whether the JIT should sign-extend an add.ovf.un operand, I think that is inconsistent with both the existing jit64 behavior, as well as the spec. The spec describes the .un instructions in terms of how they interpret their operands. From my perspective, these should behave as if they were methods taking an unsigned value as a parameter. According to the signature matching rules (table III.9), zero extension is used for unsigned arguments. It is perhaps unfortunate that there isn't an add.un analog, but the desired behavior can always be achieved by appropriate casts.

Regarding the strange behavior of the importer and x64 codegen, I think that the right approach is the first option @mikedn mentions above (i.e. keep the existing x64 behavior and zero extend add.ovf.un operands):

• make the importer set GTF_UNSIGNED when needed (this will make ARM64 codegen generate the required code)
• change x64 codegen to ignore the destination type when selecting INS_movsxd

@JosephTremoulet - thoughts, since you were of the opinion that we should change the zero-extending behavior?