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

[Chandler Carruth via llvm-commits]

chandlerc added a comment.

I'm happy to say that the successor/predecessor stuff should be correct. The last pass I'm aware of calling these is MachineBlockPlacement which has pretty clear reliance on the CFG being accurate.

If you want to double check, I actually think Quentin or Matze probably have the most context on the machine CFG at this point (more than I do honestly).

I think the only thing I'd really suggest here is to really tighten up the testing. The code and logic looks really fantastic.


================
Comment at: test/CodeGen/X86/block-placement.ll:470
@@ -468,2 +469,3 @@
+; fall-through.
 ; CHECK: fpcmp_unanalyzable_branch
 ; CHECK: %entry
----------------
Here and elsewhere, when updating a test, it would be good to convert it to use CHECK-LABEL at least, and then in the specific test cases, actually write narrow checks. For cases where we have very microscopic functions testing single instruction sequences, using update_llc_test_checks.py is incredibly useful. You can also look at the style of tests in generates and generate comparably structured checks for more complex test cases.

================
Comment at: test/CodeGen/X86/block-placement.ll:474
@@ -470,1 +473,3 @@
+; CHECK: %if.then
+; CHECK: %if.end
 ; CHECK: %exit
----------------
Especially, the checks just on these %foo terms makes it hard to at a glance see what is being checked. Having a bit more syntax, or locating the checks inside the code itself, i think will amke it much more clear.

================
Comment at: test/CodeGen/X86/fp-une-cmp.ll:79-83
@@ -79,3 +78,7 @@
 
-!1 = !{!"branch_weights", i32 1, i32 1000}
+; Test if the negation of the non-equality check between floating points are
+; translated to jnp followed by jne.
 
+; CHECK: jne
+; CHECK-NEXT: jnp
+define void @foo(float %f) {
----------------
These checks don't really make sense to me. Why are they above the function, but the function has a CHECK-LABEL adn seemingly similar checks?


http://reviews.llvm.org/D11393