[PATCH] AArch64: Use CMP;CCMP sequences for and/or/setcc trees.

[James Molloy]

Hi Matthias,

Thanks for working on this, this looks great!

I have some comments below. The patch is also a bit big and touches different areas (ISel & tablegen) but I see there is no real way to test just the tablegen changes in isolation so I think this is OK.

I'd really like Tim to cast an eye over this before it goes in, so LGTM but hold off for his.

Cheers,

James


REPOSITORY
  rL LLVM

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:771
@@ -767,1 +770,3 @@
+  case AArch64ISD::CCMP:              return "AArch64ISD::CCMP";
+  case AArch64ISD::CCMN:              return "AArch64ISD::CCMN";
   case AArch64ISD::FCMP:              return "AArch64ISD::FCMP";
----------------
Have you missed FCCMP out here?

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:1094
@@ -1088,3 +1093,3 @@
   if (VT.isFloatingPoint())
-    return DAG.getNode(AArch64ISD::FCMP, dl, VT, LHS, RHS);
+    return DAG.getNode(AArch64ISD::FCMP, dl, MVT_CC, LHS, RHS);
 
----------------
Was this broken before, or are you changing something, or are you just making something more explicit?

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:1140
@@ +1139,3 @@
+  else if (RHS.getOpcode() == ISD::SUB &&
+           isa<ConstantSDNode>(RHS.getOperand(0)) &&
+           cast<ConstantSDNode>(RHS.getOperand(0))->isNullValue() &&
----------------
This would probably look nicer with the cast to ConstantSDNode taken out of the if:

    auto *C = dyn_cast<ConstantSDNode>(RHS.getOperand(0));
    if (...)
    else if (RHS.getOpcode() == ISD::SUB && C && C->isNullValue() ...

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:1154
@@ +1153,3 @@
+/// Returns true if @p Val is a tree of AND/OR/SETCC operations.
+static bool isConjunctionDisjunctionTree(const SDValue Val) {
+  if (!Val.hasOneUse())
----------------
I'm wondering whether this should have a maximum recursion depth. I can imagine it entirely possible that some auto-generated code could produce a *huge* conjunction ladder, and might cause us to overflow here.

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:1195
@@ +1194,3 @@
+    // Determine OutCC and handle FP special case.
+    if (isInteger)
+      OutCC = changeIntCCToAArch64CC(CC);
----------------
Need brackets around this one-liner as the else case has brackets.

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.h:503
@@ -497,1 +502,3 @@
+
+  bool shouldNormalizeToSelectSequence(LLVMContext&, EVT) const override;
 };
----------------
& binds to the right.

http://reviews.llvm.org/D8232

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/