Implement integer to long/ulong casts for x86

[mikedn]

Implement various integer to long/ulong casts for x86. Sign extension is performed using an arithmetic shift to avoid the need for the x86 specific cdq. Overflow checks are implemented by using int->uint casts for which codegen provides the necessary overflow support.

Fixes #4664, #4174, #4175

[mikedn]

@BruceForstall In addition to the sign extension issue I also added support for casts with overflow checks, initially I wasn't sure how well it will work but it seems easy enough.

[mikedn]

I'm not sure this is the proper fix. The issue is that the hiStore tree lacks the GTF_EXCEPT flag that overflow checks add and an assert fires. Simply copying all flags appears to work fine.

[CarolEidt]

I think that the right fix is hiStore->gtFlags |= (tree->gtFlags & (GTF_ALL_EFFECT|GTF_LIVENESS_MASK));

[BruceForstall]

Looks like you have a few relevant test failures in the x86 ryujit suite.

[BruceForstall]

I'll look (might be tomorrow).

Also, @adiaaida PTAL
/cc @dotnet/jit-contrib

[michellemcdaniel]

Why aren't we using srcOp here?

[mikedn]

@adiaaida fgInsertEmbeddedFormTemp replaces the operand, probably I should add a srcOp = tree->gtGetOp1() right after fgInsertEmbeddedFormTemp for clarity. Or just get rid of srcOp completely.

[mikedn]

The assert that fires in test JIT\Regression\JitBlue\DevDiv_200492\DevDiv_200492\DevDiv_200492.cmd is interesting. The generated code looks like:

G_M30135_IG01:

G_M30135_IG02:
       33C0         xor      eax, eax
       50           push     eax                                   ; push loResult
       83FA00       cmp      edx, 0                                ; overflow check for hiResult
       7C0A         jl       SHORT G_M30135_IG04                   ; exit bb
       8BC2         mov      eax, edx
       50           push     eax                                   ; push hiResult
       FF15FC7BF902 call     [TestUlongAttribute:Store(long):this]

G_M30135_IG03:
       C3           ret      

G_M30135_IG04:
       E87199AA50   call     CORINFO_HELP_OVERFLOW
       CC           int3     

I doubt that it's valid to throw when esp doesn't point to the current frame and that's probably what the assert is trying to tell. Hoisting the overflow check out of the call is unlikely to work in the general case as that will affect evaluation order so I guess that such methods require a ebp frame.

[mikedn]

Test JIT\Methodical\fp\exgen\10w5d_cs_do\10w5d_cs_do.cmd is a nightmare. It fails left and right depending on options such as JitInline and JitMinOpts. For example, if I use JitMinOpts=1 I get some code like the following:

       BA02000000   mov      edx, 2
       E8F993E8FC   call     System.Single[,]:Get(int,int):float:this
       D95DF4       fstp     dword ptr [ebp-0CH]
       F30F1045F4   movss    xmm0, dword ptr [ebp-0CH]
       F30F5CC8     subss    xmm1, xmm0          ; xmm1 wasn't defined anywhere
       F30F114DF4   movss    dword ptr [ebp-0CH], xmm1

This might have something to do with float/double returns but I don't know what this has to do with my cast changes. The method where this happens doesn't even use long operations.

[BruceForstall]

Well, RyuJIT x86 is pretty raw...

Try using COMPLUS_AltJit=foo to only compile foo with RyuJIT/x86. Use COMPLUS_JitFunctionTrace=1 and COMPLUS_JitFuncInfoLogFile=log.txt to see what functions hit NYI and which don't. Sometimes it's confusing because RyuJIT/x86 with optimization will inline something and succeed, but without optimization will hit an NYI.

[mikedn]

For now I created #5047 to deal with the bad float return issue. I'll have to double check but the nightmare test seems to pass if that issue is fixed.

[mikedn]

Found another issue with long operations while investigating JIT\Methodical\fp\exgen\10w5d_cs_do\10w5d_cs_do.cmd. Sometimes lowering replaces an add with a lea:

       81EA3BD6A42A sub      edx, 0x2AA4D63B
       B90087947F   mov      ecx, 0x7F948700
       1BC1         sbb      eax, ecx
       8D0C16       lea      ecx, [esi+edx] 
       8BD7         mov      edx, edi
       13D0         adc      edx, eax
       8BC1         mov      eax, ecx

[mikedn]

Tests pass if workarounds are added for the 3 unrelated bugs I've found. However, this implementation is still flawed, the following test asserts:

class Program
{
    int i;

    [MethodImpl(MethodImplOptions.NoInlining)]
    ulong Test(int h)
    {
        int x = h + 4;
        ulong f = checked((ulong)unchecked(x - i));
        if (i > h)
            return 0;
        return f;
    }

    static void Main()
    {
        new Program().Test(42);
    }
}

Test is adapted from the test I made for the div/mod assert so the problem is likely similar - using fgInsertEmbeddedFormTemp while lowering/decomposing.

[mikedn]

Turns out that the above issue is caused by a bug in GT_STORE_LCL_VAR decomposition. It tries to insert a new statement after the current one but the current statement has an embedded statement. The new statement ends up being inserted between the current statement and its embedded statement.

How do we move forward with all this stuff? Attempting to add support for long casts uncovered 4 unrelated issues. Probably the same thing will happen if we try to add support for other long operations.

[CarolEidt]

Why is this unconditionally setting the frame type to EBP?

[mikedn]

Temporary workaround to get JIT\Regression\JitBlue\DevDiv_200492\DevDiv_200492\DevDiv_200492.cmd working, see explanation a few posts above. The correct solution is to detect when an EBP frame is required for x86. Probably I should create an issue for this.

Obviously, those workarounds need to be removed if this is to be merged.

[CarolEidt]

@mikedn - Given where we are with supporting longs on 32-bit, I think that the only near-term way forward is to deal with each of these issues in turn; I wish I could give a more definitive answer, but I think some of what we are doing is postulating a way forward, and then discovering the issues.
For the GT_STORE_LCL_VAR issue, I suspect the answer is possibly messy, but straightforward - we need to ensure that the embedded case is correctly handled. For the lea issue, I think the right answer is to have a flag something like GTF_FLAGS that indicates that these nodes have a flag dependency. (Ouch - too confusing talking about two kinds of flags.) Setting that would also allow us to avoid generating an lea in that case.

[mikedn]

Given where we are with supporting longs on 32-bit, I think that the only near-term way forward is to deal with each of these issues in turn; I wish I could give a more definitive answer, but I think some of what we are doing is postulating a way forward, and then discovering the issues.

I suppose separate issues should be created and addressed individually rather than attempting to fix everything in a single PR?

For the GT_STORE_LCL_VAR issue, I suspect the answer is possibly messy, but straightforward - we need to ensure that the embedded case is correctly handled.

Might be easier to always put the hiStore in an embedded statement before the loStore, even if this means that those stores don't follow the usual lo/hi order. Anyway I'm not sure how the current code works at all, it would seem that dragging hiRhs in a separate statement would cause a lot of problems if hiRhs is not a leaf.

For the lea issue, I think the right answer is to have a flag something like GTF_FLAGS that indicates that these nodes have a flag dependency.

Or adding a GT_ADD_LO.

[mikedn]

For the lea issue, I think the right answer is to have a flag something like GTF_FLAGS that indicates that these nodes have a flag dependency.

Or perhaps we can piggyback on the existing but presently unused GTF_SET_FLAGS. And perhaps later that one can also be used to eliminate the unnecessary 0 compare from code like if (x + 42 == 0).

[CarolEidt]

I hadn't realized that GTF_SET_FLAGS existed. I think that would be a good choice.

I suppose separate issues should be created and addressed individually rather than attempting to fix everything in a single PR?

That would be great. Without your changes, can you duplicate the GT_STORE_LCL_VAR issue (even if it doesn't result in an assert or bad codegen)?

[mikedn]

Without your changes, can you duplicate the GT_STORE_LCL_VAR issue (even if it doesn't result in an assert or bad codegen)?

See #5696

[pgavlin]

The store nodes should also have the GTF_ASG flag set. We should probably validate that all stores have this flag set in CheckLIR, but that change should be made separately.

[pgavlin]

I think this is very close: the changes at https://gist.github.com/pgavlin/77e2364408d6e63223b19319757c2170 take care of the cost-related asserts.

[mikedn]

@pgavlin Are the cost related changes still needed? If I understand correctly the costs are no longer needed since #6849 but I see that decomposition still attempts to copy costs here and there.

[pgavlin]

@mikedn: the cost changes should no longer be necessary, no. If you notice any parts of the backend that still attempt to maintain cost info, those parts should be fixed.

[pgavlin]

(indeed, attempting to maintain cost information may even cause asserts if the source node was added or modified by rationalize, which no longer sets or maintains cost info)

[mikedn]

The failure of JIT\Performance\CodeQuality\BenchI\Pi\Pi\Pi.cmd appears to be caused by a bug in long compare codegen. The generation of the compare is delayed until GT_JTRUE but that only works correctly if the jump follows the compare. In the case of PI benchmark LSRA appears to have inserted 2 reloads between the compare and the jump and those 2 reloads modify registers that the compare is supposed to be using.

[mikedn]

Well, this one is ready provided that we decide what to do about the failing test. It may sound odd but I'd fix the test because that long comparison is probably an accident. #7038 contains a much simpler repro that we can later add as a separate test.

[pgavlin]

Nit: might be nice to have a note in here that this is a conversion (u32 -> u64) that cannot overflow.

[pgavlin]

LGTM aside from a couple nits re: comments.

[BruceForstall]

@mikedn Nice! Thanks for the work!