RyuJIT x86: Lowering sometimes replaces ADD with LEA when ADD is required to set flags

[mikedn]

Found in dotnet/coreclr#4986. JIT\Methodical\fp\exgen\10w5d_cs_do\10w5d_cs_do.cmd fails because an ADD used by low part of a long ADD operation was replaced with a LEA:

81EA3BD6A42A sub      edx, 0x2AA4D63B
B90087947F   mov      ecx, 0x7F948700
1BC1         sbb      eax, ecx
8D0C16       lea      ecx, [esi+edx]  ; the carry flag is required by the adc below
8BD7         mov      edx, edi
13D0         adc      edx, eax
8BC1         mov      eax, ecx

[mikedn]

Possible solutions:

• add a new GT flag to mark the operation as requiring carry flag to be set
• use the existing GTF_SET_FLAGS
• add GT_ADD_LO

Using the existing GTF_SET_FLAGS requires FEATURE_SET_FLAGS to be enabled. It is already enabled on ARM but not on x86/x64. The current behavior on x86/x64 seems dubious - #4962.

[mikedn]

Also, codegen sometimes generates INC/DEC instead of ADD/SUB and INC/DEC do not set the carry flag. GTF_SET_FLAGS should probably be repurposed to mean "set carry flag" while the existing GTF_ZSF_SET keeps its meaning.

[mikedn]

Simple test case:

[MethodImpl(MethodImplOptions.NoInlining)]
static long Test(long x) {
    return x + 1;
}
static void Main() {
    Console.WriteLine(Test(uint.MaxValue).ToString());
}

Prints 0 which is obviously wrong.

[mikedn]

@RussKeldorph Anyone working on this? I'll give it a try using GT_ADD_LO/GT_SUB_LO to see what the team thinks. To me it seems easier and safer than using GTF_SET_FLAGS.

[RussKeldorph]

@mikedn I don't think so. Take it.
@adiaaida @BruceForstall @CarolEidt FYI

[CarolEidt]

@mikedn - I agree that adding GT_ADD_LO / GT_SUB_LO seems like a cleaner solution. I'm not sure whether that might change when/if we add the optimization to reuse flags, but one could always change this if it was then felt to be duplicative.