[PATCH] D101074: [X86] Canonicalize SGT/UGT compares with constants to use SGE/UGE to reduce the number of EFLAGs reads. (PR48760)

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Some thoughts.

> Some min/max-like patterns lose out because the comparison constant is now different from the select value - do we have any opinions on how critical this is?

This does seem not ideal. Can we even check that the cmp has a select with such an operand by now?



================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:23462-23463
   if (Op0.getSimpleValueType().isInteger()) {
+    // Attempt to canonicalize SGT/UGT compares with constants to use
+    // SGE/UGE instead - which will reduce the number of EFLAGs reads.
+    if (auto *Op1C = dyn_cast<ConstantSDNode>(Op1)) {
----------------
Why do we handle inverse case of decrementing in `combineSetCCAtomicArith()` but not here?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:23465-23470
+      const APInt &Op1Val = Op1C->getAPIntValue();
+      if (!Op1Val.isNullValue() &&
+          ((CC == ISD::CondCode::SETGT && !Op1Val.isMaxSignedValue()) ||
+           (CC == ISD::CondCode::SETUGT && !Op1Val.isMaxValue()))) {
+        // Only do this if incrementing the constant doesn't increase the bit
+        // encoding size.
----------------
This comment reads as-if it's about the previous condition.
I'd suggest adding something like "note that we can't change GT to GE for tautological comparisons,
where incrementing the limit would cause an overflow" before `if (!Op1Val.isNullValue() &&`


================
Comment at: llvm/test/CodeGen/X86/sdiv_fix_sat.ll:478-484
+; X86-NEXT:    cmpl $-2147483647, %esi # imm = 0x80000001
+; X86-NEXT:    movl $-2147483648, %ecx # imm = 0x80000000
+; X86-NEXT:    cmovael %esi, %ecx
+; X86-NEXT:    movl $0, %edx
+; X86-NEXT:    cmovael %eax, %edx
 ; X86-NEXT:    cmpl $-2147483648, %esi # imm = 0x80000000
+; X86-NEXT:    cmovel %eax, %edx
----------------
This is the clamp regression i guess?


================
Comment at: llvm/test/CodeGen/X86/sdiv_fix_sat.ll:634-635
+; X64-NEXT:    cmovnsq %rcx, %rbp
+; X64-NEXT:    movabsq $-4294967295, %rax # imm = 0xFFFFFFFF00000001
+; X64-NEXT:    cmpq %rax, %r13
 ; X64-NEXT:    movabsq $-4294967296, %rcx # imm = 0xFFFFFFFF00000000
----------------
same elesewhere in the file


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101074/new/

https://reviews.llvm.org/D101074