[PATCH] D19261: X86 _comi_ intrinsics - Fixed lowering

David Kreitzer via llvm-commits llvm-commits at lists.llvm.org
Mon May 2 07:36:02 PDT 2016


DavidKreitzer added a comment.

Hi Elena,

Modulo a few nits, the code changes look good now. Just one additional request for improving the tests.

Also, could you please avoid updating your sources between revisions unless absolutely necessary? That makes it easier for your code reviewers to do incremental reviews. Alternatively, you can post a revision that consists solely of the update. The important things is that reviewers have a way to see just the changes that you've made since the prior revision.

Thanks,
Dave


================
Comment at: ../lib/Target/X86/X86ISelLowering.cpp:17476
@@ +17475,3 @@
+      switch (CC) {
+        case ISD::SETEQ: { // ZF,PF,CF <- 100
+        SetCC = DAG.getNode(X86ISD::SETCC, dl, MVT::i8,
----------------
This case is indented incorrectly.


================
Comment at: ../lib/Target/X86/X86ISelLowering.cpp:17498
@@ +17497,3 @@
+        break;
+      case ISD::SETLT: { // ZF,PF,CF <- 001 (CF = 1 and ZF = 0)
+        SetCC = DAG.getNode(X86ISD::SETCC, dl, MVT::i8,
----------------
The comment here is a bit misleading, because the code ultimately doesn't check for "ZF,PF,CF <- 001".  It would be clearer to write something like "// Reverse operands and check for GT."


================
Comment at: ../lib/Target/X86/X86ISelLowering.cpp:17507
@@ +17506,3 @@
+        break;
+      case ISD::SETLE: // (ZF=1 or CF=1) and PF=0
+        SetCC = DAG.getNode(X86ISD::SETCC, dl, MVT::i8,
----------------
Similar comment here.

================
Comment at: ../test/CodeGen/X86/sse-intrinsics-x86.ll:40
@@ -38,3 +39,3 @@
 define i32 @test_x86_sse_comige_ss(<4 x float> %a0, <4 x float> %a1) {
   ; CHECK: comiss
   ; CHECK: setae
----------------
I like changes you made to test for an explicit operand order for [u]comiss in some places. Could you do that here too and for all the tests where it matters (i.e. gt, ge, lt, and le)? Also for sse2-intrinsics-x86.ll, please.



Repository:
  rL LLVM

http://reviews.llvm.org/D19261





More information about the llvm-commits mailing list