[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: llvm/trunk/lib/Target/X86/X86InstrInfo.h:57
@@ +56,3 @@
+  COND_NE_OR_P,
+  COND_NP_OR_E,
+
----------------
DavidKreitzer wrote:
> congh wrote:
> > DavidKreitzer wrote:
> > > The COND_NP_OR_E condition is rather pointless. Based on the comment, it is pretty clear that the intent was for this to be an artificial condition for FCMP_OEQ, but that should be COND_NP_AND_E. (AND not OR)
> > > 
> > > In other words, I'd recommend deleting the existing COND_NP_OR_E condition and the COND_P_AND_NE one that you added. COND_NE_OR_P and its inverse COND_E_AND_NP are sufficient.
> > > 
> > > [FWIW, COND_NP_OR_E would always evaluate to true assuming the CC's originated from an FP compare instruction like COMISS.]
> > This makes sense. Done.
> I would recommend combining these two comments. As written, they are a little misleading, because they suggest that COND_NE_OR_P is used to implement both FCMP_OEQ & FCMP_UNE while COND_E_AND_NP is used just to negate COND_NE_OR_P. In fact, COND_NE_OR_P is the natural implementation of FCMP_UNE while COND_E_AND_NP is the natural implementation of FCMP_OEQ. How about something like this?
> 
> 
> ```
> // Artificial condition codes. These are used by AnalyzeBranch
> // to indicate a block terminated with two conditional branches that together
> // form a compound condition. They occur in code using FCMP_OEQ or FCMP_UNE,
> // which can't be represented on x86 with a single condition. These
> // are never used in MachineInstrs and are inverses of one another.
> COND_NE_OR_P,
> COND_E_AND_NP,
> ```
Your suggested comment looks great! I have updated the patch accordingly. Thanks!


http://reviews.llvm.org/D11393