[PATCH] D11393: [X86] Allow X86::COND_NE_OR_P and X86::COND_NP_OR_E to be reversed.

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:4037
@@ -4037,1 +4036,3 @@
+                              "destinations for two branch instructions.");
       BranchCode = X86::COND_NE_OR_P;
+    } else if ((OldBranchCode == X86::COND_NE && BranchCode == X86::COND_NP) ||
----------------
There is no need to change anything between line 4028 and 4037 (to simplify the patch). Just add a combined assertion after the pattern recognition:

assert( (BranchCode != cond_np_or_e || BranchCode != cond_ne_or_p || NewTB == TBB) && "Identical target BB expected");

Actually since the previous early exit has been removed, the assert can fire off, so the right thing to do is add the TBB == newTBB condition

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:4039
@@ +4038,3 @@
+    } else if ((OldBranchCode == X86::COND_NE && BranchCode == X86::COND_NP) ||
+               (OldBranchCode == X86::COND_P && BranchCode == X86::COND_E)) {
+      // X86::COND_P_AND_NE usually has two different branch destinations.
----------------
Should condition NewTBB != TBB be added here too?

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:4056
@@ +4055,3 @@
+      // Similarly it branches to B2 only if NP && E. That is why this condition
+      // is named COND_P_AND_NE.
+      BranchCode = X86::COND_P_AND_NE;
----------------
It is confusing here. The comment says the condition to B2 is NP_AND_E, but the branch code is P_AND_NE ..

Also the condition to B1 is NE_OR_P, so why not using COND_NE_OR_P?

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:4059
@@ +4058,3 @@
+    } else if ((OldBranchCode == X86::COND_NP && BranchCode == X86::COND_NE) ||
+               (OldBranchCode == X86::COND_E && BranchCode == X86::COND_P)) {
+      // See comments above for X86::COND_P_AND_NE.
----------------
should NewTBB == TBB be added here?

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:4065
@@ +4064,3 @@
+      // destination except X86::COND_P_AND_NE and X86::COND_E_AND_NP.
+      if (TBB != NewTBB)
+        return true;
----------------
Why not unconditionally return true in else {} ?

================
Comment at: lib/Target/X86/X86InstrInfo.h:66
@@ +65,3 @@
+  // never used in MachineInstrs.
+  COND_E_AND_NP,
+  COND_P_AND_NE,
----------------
add a comment  after the enum:

COND_E_AND_NP, // negate of COND_NE_OR_P

'AND' does not directly map to any branch patterns, so add the comment help understanding the semantics.


http://reviews.llvm.org/D11393