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

[Gerolf Hoflehner]

Gerolf added a comment.

"Actually this optimization is also done in block-placement pass, which means we can remove this optimization from AnalyzeBranch()." Does have AnalyzeBranch() have more clients or just block placement? In that case moving the code may impact generated code.


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:3530
@@ -3568,3 +3529,3 @@
     // we could handle more patterns here, but we shouldn't expect to see them
     // if instruction selection has done a reasonable job.
     if ((OldBranchCode == X86::COND_NP &&
----------------
It would be nice if you added a FIXME even though this is not  part of your code. Any assumption about the IS patterns  should be made explicit and checked with  an assertion.

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:3541
@@ -3579,1 +3540,3 @@
       BranchCode = X86::COND_NE_OR_P;
+    else if ((OldBranchCode == X86::COND_NE &&
+              BranchCode == X86::COND_NP) ||
----------------
Perhaps I'm only iterating what Duncan said. What I'm confused by is that the previous pattern are symmetrical: For example the first case is NP && E or E && NP while the new cases are asymmetrical like here NE && NP or P && E (as opposed to  NE && NP or NP && NE, which is what I would expect from the handling of the previous pattern). At least there need to be a good explanation (comment) for this.

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:3554
@@ -3582,1 +3553,3 @@
 
+    // Only handle the case where all conditional branches branch to the same
+    // destination except X86::COND_P_AND_NE and X86::COND_E_AND_NP, which have
----------------
I would need a picture and examples to understand for which conditions chains the TBB condition is relevant. 


http://reviews.llvm.org/D11393