[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 a comment.

In http://reviews.llvm.org/D11393#220180, @Gerolf wrote:

> In addition to the comments what about ReverseBranchCondition? Shouldn't the new opcodes be handled there, too?


In ReverseBranchCondition(), GetOppositeBranchCondition() is called to get the reverse condition's opcode, and this function is updated in this patch to return the correct reverse condition for COND_NE_OR_P/COND_NP_OR_E/COND_E_AND_NP/COND_P_AND_NE.

> Perhaps it would be best if you worked directly with the code owner and get his consent.


OK. I found Nadav Rotem is the code owner of X86 backend. I will add him as a reviewer.


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:3547
@@ +3546,3 @@
+      // JNE B1
+      // JNP B2
+      // B1: (fall-through)
----------------
Gerolf wrote:
> 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.
This is because on X86 the equality/non-equality comparison between floating points is translated into two instructions, and the conditions of those two instructions represent a logical OR instead of AND, as they jump to the same destination. Normally the negation of OR is AND, and that is why the reverse condition is named with AND. I agree that it is not straightforward to understand that it has two different branch targets, but that is the correct way to reverse it. 


http://reviews.llvm.org/D11393