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

[David Kreitzer via llvm-commits]

DavidKreitzer added a comment.

Ayal Zaks asked me to review this patch after he was asked by Nadav.

The stability failures caused by this patch are most likely caused by one or both of the issues in X86InstrInfo::AnalyzeBranchImpl. Cleaning up the unnecessary conditions would be a nice bonus. The inefficiency in fp-une-cmp.ll might be a separate issue.


================
Comment at: llvm/trunk/lib/Target/X86/X86InstrInfo.cpp:4033
@@ -4033,3 +4032,3 @@
     X86::CondCode OldBranchCode = (X86::CondCode)Cond[0].getImm();
     if (OldBranchCode == BranchCode)
       continue;
----------------
Don't you need to check TBB == NewTBB here also?

================
Comment at: llvm/trunk/lib/Target/X86/X86InstrInfo.cpp:4071
@@ +4070,3 @@
+      // See comments above for X86::COND_P_AND_NE.
+      BranchCode = X86::COND_E_AND_NP;
+    } else
----------------
I think you need to verify here that NewTBB == FBB before making this transformation. If FBB is nullptr, you'll need to compute the fallthrough block and check that it matches NewTBB.


================
Comment at: llvm/trunk/lib/Target/X86/X86InstrInfo.h:57
@@ +56,3 @@
+  COND_NE_OR_P,
+  COND_NP_OR_E,
+
----------------
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.]

================
Comment at: llvm/trunk/test/CodeGen/X86/fp-une-cmp.ll:51
@@ +50,3 @@
+; CHECK:       %bb1
+; CHECK:       jmp
+;
----------------
If we invert the compound branch at the end of the entry block and place bb1 before bb2, we can eliminate the jmp at the end of bb1. Do you know why that isn't happening?



Repository:
  rL LLVM

http://reviews.llvm.org/D11393