[PATCH] D138349: [X86][NFC] Minor improvement in X86InstrInfo::optimizeCompareInstr

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:4501
     // EFLAGS is used by this instruction.
-    X86::CondCode OldCC = X86::COND_INVALID;
-    if (MI || IsSwapped || ImmDelta != 0) {
-      // We decode the condition code from opcode.
-      if (Instr.isBranch())
-        OldCC = X86::getCondFromBranch(Instr);
-      else {
-        OldCC = X86::getCondFromSETCC(Instr);
-        if (OldCC == X86::COND_INVALID)
-          OldCC = X86::getCondFromCMov(Instr);
-      }
-      if (OldCC == X86::COND_INVALID) return false;
-    }
+    X86::CondCode OldCC = X86::getCondFromMI(Instr);
+    if ((MI || IsSwapped || ImmDelta != 0) && OldCC == X86::COND_INVALID)
----------------
skan wrote:
> pengfei wrote:
> > Maybe they are not equal? `getCondFromMI` checks for `isJCC`, `isSETCC` and `isCMOVCC`, but `getCondFromBranch` only checks `JCC_1`.
> They should be equal. There is neither JCC_2 nor JCC_4 before assembler relaxation if the MIR is not written manually . Furthermore, if  someone write the the MIR with JCC_2, JCC4 manually, the optimization can apply too.
OK. Is it possible we get a valid CC but `MI || IsSwapped || ImmDelta != 0` is false? I'd suggest below for conservative.
```
  X86::CondCode OldCC = X86::COND_INVALID;
  if (MI || IsSwapped || ImmDelta != 0) {
    OldCC = X86::getCondFromMI(Instr);
    if (OldCC == X86::COND_INVALID)
      return false;
  }
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138349