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

[Kan Shengchen via Phabricator via llvm-commits]

skan 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)
----------------
pengfei wrote:
> 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;
>   }
> ```
Good question! You suggested code is absolutely right. After looking into these code, I found that `OldCC` is used only when `ShouldUpdateCC` is true and `ShouldUpdateCC` is set to true only when `MI || IsSwapped || ImmDelta != 0` is true.  So we can simply your code further, namely, my code should be correct too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138349