[PATCH] Add intrinsics and SDNodes for signed/unsigned absolute difference (absdiff) and horizontal add (hadd).

[James Molloy]

Hi,

Thanks for working on this!

I have a bunch of comments, and this is just a first round review. Also, this needs to be split into three separate patches:

1. Addition of sabsdiff/uabsdiff
2. Addition of hadd
3. X86 changes

Each should have testcases. The first two don't currently; you're only testing the X86 backend's handling of the intrinsics which may be fine, but you do need to ensure you're checking the generic EXPAND behaviour as well as the specific X86 behaviour you've added in patch (3).

Cheers,

James


REPOSITORY
  rL LLVM

================
Comment at: docs/LangRef.rst:9562
@@ +9561,3 @@
+
+'``llvm.uabsdiff.*``' and '``llvm.sabsdiff.*``' Intrinsics
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
----------------
I don't think floating point datatypes should be bundled in with integer datatypes in these intrinsics. Signedness isn't a concept for floating point datatypes, so it would be weird to have something operate on floats with a defined signedness. Also, it would mean two intrinsics were identical for floating point types which raises a canonicalisation problem.

I'd be happy if the floating point support for these intrinsics was removed for the time being, if you don't want to add another intrinsic right now.

================
Comment at: docs/LangRef.rst:9586
@@ +9585,3 @@
+
+    These intrinsics are primarily used during optimization phase.
+
----------------
What does this note mean, in terms of action required by the reader? Do you mean that users should aim not to write these intrinsics? If so, what should they do instead?

================
Comment at: docs/LangRef.rst:9619
@@ +9618,3 @@
+
+'``llvm.uhadd.*``' and '``llvm.shadd.*``'Intrinsic
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
----------------
Because addition in wrapping 2's complement arithmetic is sign independent, you shouldn't need a signed and unsigned version of this. Just "llvm.hadd" should do.

================
Comment at: include/llvm/CodeGen/ISDOpcodes.h:335
@@ +334,3 @@
+    /// These nodes are generated from llvm.*absdiff* intrinsics.
+    SABSD, UABSD,
+
----------------
I'd prefer "SABSDIFF" here to match the intrinsic better and be more self documenting.

================
Comment at: include/llvm/Target/TargetSelectionDAG.td:388
@@ -387,1 +387,3 @@
 
+def sabsd       : SDNode<"ISD::SABSD"      , SDTIntBinOp>;
+def shadd       : SDNode<"ISD::SHADD"      , SDTIntUnaryOp>;
----------------
I'd prefer "sabsdiff" here to match the intrinsic name better, and be more self documenting.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2888
@@ +2887,3 @@
+    }
+    Results.push_back(Tmp1);
+    break;
----------------
What happens if the "if" condition fails? Tmp1 is a dangling pointer at the moment.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2893
@@ +2892,3 @@
+  case ISD::SHADD: {
+    // FIXME: Improve the expansion
+    SDValue OpVal = Node->getOperand(0);
----------------
Please write how you expect the expansion to be improved.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:160
@@ -155,1 +159,3 @@
 
+SDValue DAGTypeLegalizer::PromoteIntRes_AbsoluteDiff(SDNode *N) {
+  SDValue LHS, RHS;
----------------
I'm not sure about this - I think you shouldn't need to care about the high bits of a promoted value (that's the responsibility of PromoteIntOp of the user), so you can just use PromoteIntRes_SimpleBinOp here instead.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:908
@@ -877,1 +907,3 @@
   case ISD::ROTR: Res = PromoteIntOp_Shift(N); break;
+  case ISD::UHADD:
+  case ISD::SHADD: Res = PromoteIntOp_HADD(N); break;
----------------
Why are you not handling ISD::SABD and friends here?

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:1243
@@ -1210,1 +1242,3 @@
 
+SDValue DAGTypeLegalizer::PromoteIntOp_HADD(SDNode *N) {
+  SDValue Op;
----------------
You don't need this. Because addition is agnostic to the high bits, you don't need to extend anything. You can just use PromoteIntOp_SimpleIntBinOp here.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp:229
@@ -226,1 +228,3 @@
   case ISD::SRL_PARTS:                  return "srl_parts";
+  case ISD::UABSD:                       return "uabsd";
+  case ISD::SABSD:                       return "sabsd";
----------------
Indentation error.

http://reviews.llvm.org/D10273

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