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

[Gerolf Hoflehner via llvm-commits]

Gerolf added a comment.

In addition to the comments what about ReverseBranchCondition? Shouldn't the new opcodes be handled there, too?
Perhaps it would be best if you worked directly with the code owner and get his consent.


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:3530
@@ -3568,2 +3529,3 @@
     // we could handle more patterns here, but we shouldn't expect to see them
     // if instruction selection has done a reasonable job.
+    auto NewTBB = I->getOperand(0).getMBB();
----------------
I guess my comment was a bit too out the box. What I had in mind is  not related to your review.  So let's table this.

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:3532
@@ +3531,3 @@
+    auto NewTBB = I->getOperand(0).getMBB();
+    if ((OldBranchCode == X86::COND_NP && BranchCode == X86::COND_E) ||
+        (OldBranchCode == X86::COND_E && BranchCode == X86::COND_NP)) {
----------------
Now that the NewTBB check has been removed potentially the assertions below can fire.

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:3547
@@ +3546,3 @@
+      // JNE B1
+      // JNP B2
+      // B1: (fall-through)
----------------
The problem I have with this review is partially historical. COND_NE_OR_P etc doesn't make sense to me without explanation. OR does not seem to be used in a logical sense since both branch conditions should be present. But also your COND names obfuscate the picture (perhaps just for me though) even more: previously the branch targets had to be identical, now they can be different. This is a new concept and pressing it into the existing AnalyzeBranch routine makes the code harder too maintain. From this angle to have functions that handle the new functionality.


http://reviews.llvm.org/D11393