Expand GT_JCC/SETCC condition support

[mikedn]

Currently GT_JCC/SETCC nodes use relops as condition codes and this prevents the use of flag specific conditions (e.g. S, NS, P, NP) and floating point conditions. Additionally, the code used by GT_JTRUE and relop codegen is rather convoluted and not very easy to reuse for GT_JCC/SETCC codegen.

This changes GT_JCC/SETCC's condition to GenCondition, a class that can encode all necessary conditions - signed/unsigned integral conditions, ordered/unordered floating point conditions and flag specific conditions. Mapping between GenCondition codes and emitter jump kinds is done via arch specific tables instead of genJumpKindForOper and genJumpKindsForTree.

GT_JTRUE codegen has been updated to also use GenCondition so that genJumpKindForOper and genJumpKindsForTree can be removed.

Contributes to #17073

[mikedn]

This produces some small jit diffs:

; x64
Total bytes of diff: -3 (0.00% of base)
    diff is an improvement.
Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes
Top file improvements by size (bytes):
          -3 : System.Data.Common.dasm (0.00% of base)
1 total files with size differences (1 improved, 0 regressed), 129 unchanged.
Top method improvements by size (bytes):
          -3 : System.Data.Common.dasm - DataTable:BeginLoadData():this
1 total methods with size differences (1 improved, 0 regressed), 145666 unchanged.

; x86
Total bytes of diff: -45 (0.00% of base)
    diff is an improvement.
Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes
Top file improvements by size (bytes):
         -36 : Microsoft.CodeAnalysis.CSharp.dasm (0.00% of base)
          -3 : NuGet.Protocol.Core.v3.dasm (0.00% of base)
          -3 : System.Data.Common.dasm (0.00% of base)
          -3 : System.Threading.Tasks.Dataflow.dasm (0.00% of base)
4 total files with size differences (4 improved, 0 regressed), 126 unchanged.
Top method improvements by size (bytes):
         -36 : Microsoft.CodeAnalysis.CSharp.dasm - Binder:FoldNeverOverflowBinaryOperators(int,ref,ref):ref
          -3 : NuGet.Protocol.Core.v3.dasm - SafeBoolConverter:ReadJson(ref,ref,ref,ref):ref:this
          -3 : System.Data.Common.dasm - DataTable:BeginLoadData():this
          -3 : System.Threading.Tasks.Dataflow.dasm - WriteOnceBlock`1:get_DebuggerDisplayContent():ref:this
4 total methods with size differences (4 improved, 0 regressed), 145649 unchanged.

All diffs are due to a change in setcc codegen - the old genCodeForSetcc ignored the node type and always emitted a movzx. genSetRegToCond emitted a movzx only when the type was not byte. The new inst_SETCC behaves like genSetRegToCond and emits a movzx only when necessary.

[mikedn]

In an attempt to get better validation for this change (the JIT doesn't currently produce a lot of JCC/SETCC nodes) I made various experiments:

• lower all relops to CMP/TEST/SETCC/JCC
• remove redundant 0 test from x + y < 0 by using S condition
• remove redundant equality test from FP x == x by using P condition
• lower HW intrinsic nodes that set condition flags to intrinsic + SETCC/JCC (#17073)
• generate CMOVcc via if-conversion

Code can be found in https://github.com/mikedn/coreclr/commits/cc-cond2-exp. It's not 100% complete/accurate/correct etc. but at least tests pass.

[briansull]

Very Nice!

[briansull]

The Intel arch does define both jp and jpe, I don't think that we even use these instructions but it seems to me that jpe and jpo are more understandable than jp and jnp

https://c9x.me/x86/html/file_module_x86_id_146.html

[mikedn]

IMO jpe/jpo are more understandable when used for the original purpose - parity checking. But that's history now and the only purpose of the PF flag these days is detecting "unordered" floating point comparisons. In this case jp/jnp seem preferable because the clearly indicate that the branch is taken when PF=1/0.

[mikedn]

VS's own disassembler also uses jp/jnp:

00007FFD026C049F C4 E1 7B 10 05 E8 42 EF FF vmovsd      xmm0,qword ptr [7FFD025B4790h]  
00007FFD026C04A8 C4 E1 79 2E 05 E7 42 EF FF vucomisd    xmm0,mmword ptr [7FFD025B4798h]  
00007FFD026C04B1 7A 02                jp          00007FFD026C04B5  
00007FFD026C04B3 74 0B                je          00007FFD026C04C0  

[briansull]

OK I am good with this change.
Thanks for the explaination.

[briansull]

Looks Good

[AndyAyersMS]

This is also lays the groundwork for generating CMOV, right?

[mikedn]

This is also lays the groundwork for generating CMOV, right?

I wouldn't say it's groundwork but it certainly helps. Without it, CMOV would likely be more limited (e.g. no floating point conditions or no ability to handle condition flags set by BT).

Of course, none of this deals with the thorny question of representing condition flags data flow explicitly. But unless the JIT starts doing fancy optimizations post lowering that's not much of an issue.

[mikedn]

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

[BruceForstall]

@dotnet-bot test this please

[BruceForstall]

@briansull @AndyAyersMS Do you want to re-review this?

@mikedn Is there any reason this has been sitting for so long, except for us not being responsive? I.e., is it ready to merge, if tests pass (again)?

[mikedn]

@BruceForstall Yes, it's ready to merge.

[briansull]

Looks Good Again