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

[Cong Hou via llvm-commits]

congh 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) ||
----------------
davidxl wrote:
> 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
You are right. But in practice I think when there is COND_NP and COND_E, their target is always the same (that is why I added assertion in this patch). But to be safer, I have replaced the assertion with a check.

================
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.
----------------
davidxl wrote:
> Should condition NewTBB != TBB be added here too?
They can be the same target. This means we don't have to check if they are the same or different targets.

================
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;
----------------
davidxl wrote:
> 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?
I found the comment is incorrect. It is for COND_E_AND_NP not COND_P_AND_NE. I have updated the comment.

================
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.
----------------
davidxl wrote:
> should NewTBB == TBB be added here?
You mean NewTBB != TBB? See my comment above: they are be the same target.

================
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;
----------------
davidxl wrote:
> Why not unconditionally return true in else {} ?
I think it is OK to unconditionally return true here.


http://reviews.llvm.org/D11393