[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:4055
@@ +4054,3 @@
+      // is named COND_P_AND_NE.
+      BranchCode = X86::COND_P_AND_NE;
+    } else if ((OldBranchCode == X86::COND_NP && BranchCode == X86::COND_NE) ||
----------------
congh wrote:
> davidxl wrote:
> > congh wrote:
> > > davidxl wrote:
> > > > For B1, the condition is NP_OR_E, so why not using COND_NP_OR_E as the branch code? Is the new code needed? (after swapping TBB and FBB)
> > > This is actually why this patch is created: COND_NP_OR_E is equivalent to COND_P_AND_NE, but the latter has the second condition reversed based on the former. COND_NP_OR_E has a JNP and JE, while COND_P_AND_NE has a JNP and JNE, or JE and JP. COND_NP_OR_E always has two identical targets, but COND_P_AND_NE doesn't. So they are different patterns. We use different names so that in X86::GetOppositeBranchCondition() we can get the reverse condition code for each other.
> > I am not sure if we need to introduce the Opposite branch code for E_OR_NP.
> > 
> > Example:
> > 
> >   JE B1
> >   JP B2
> > B1 (fall through):
> > 
> > B2:
> > 
> > For this case, the Analyze branch can return success with the following tuple:
> > {BranchCode =  E_OR_NP, TBB = B1, FBB = B2}
> > 
> > Later when insertBranch is called: there are two scenarios:
> > 1) same as above where B1 is the fall through. The inserted code will be the same as above
> > 2) B2 is the layout successor.  In this second case, the generated code sequence should look like:
> >    JNP B1
> >    JE   B1
> > B2 (fall through):
> > ..
> > B1:
> > 
> > or 
> >   JE B1
> >   JNP B1
> > B2:
> >   ..
> > B1:
> > 
> > Does it make sense?
> In some places GetOppositeBranchCondition() is used to get the opposite branch code in order to generate an opposite branch. If we don't have an opposite branch code for COND_E_OR_NP, then how to reverse it?
GetOppositteBranchCondition for some reason is never called with COND_E_OR_NP before, do you know why this path was not triggered ?


http://reviews.llvm.org/D11393