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

[skan]

Summary

Before this patch, the code enumerated getCondFromBranch, getCondFromSETCC and getCondFromFromCMov to get the condition code of a MachineInstr, and assigned the result to variable OldCC when MI || IsSwapped || ImmDelta != 0 was satisfiled.

After this patch, the if-else structure is eliminated by using getCondFromMI. Since OldCC is only used when MI || IsSwapped || ImmDelta != 0 is true, it is initialized with getCondFromMI directly outside the scope of if now.

[pengfei]

llvm/lib/Target/X86/X86InstrInfo.cpp

4501–4504

Maybe they are not equal? getCondFromMI checks for isJCC, isSETCC and isCMOVCC, but getCondFromBranch only checks JCC_1.

[skan]

llvm/lib/Target/X86/X86InstrInfo.cpp

4501–4504

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.

[pengfei]

llvm/lib/Target/X86/X86InstrInfo.cpp

4501–4504

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;
}

[skan]

llvm/lib/Target/X86/X86InstrInfo.cpp

4501–4504

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.

[RKSimon]

please can you add a patch summary

[skan]

In D138349#3940029, @RKSimon wrote:

please can you add a patch summary

Sure! Done.

[pengfei]

LGTM.

[skan]

Thanks for the review @pengfei @RKSimon !

llvm/lib/Target/X86/X86InstrInfo.cpp

if (!UseEFLAGS && ModifyEFLAGS) {
       // It is safe to remove CmpInstr if EFLAGS is updated again.
       FlagsMayLiveOut = false;
       break;
     }
     if (!UseEFLAGS && !ModifyEFLAGS)
       continue;
 
     // 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)
+      return false;
+
     X86::CondCode ReplacementCC = X86::COND_INVALID;
     if (MI) {
       switch (OldCC) {
       default: break;
       case X86::COND_A: case X86::COND_AE:
       case X86::COND_B: case X86::COND_BE:
         // CF is used, we can't perform this optimization.
         return false;