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

Shahid Asghar-ahmad.Shahid at amd.com
Tue Jun 30 02:30:21 PDT 2015


REPOSITORY
  rL LLVM

================
Comment at: docs/LangRef.rst:9562
@@ +9561,3 @@
+
+'``llvm.uabsdiff.*``' and '``llvm.sabsdiff.*``' Intrinsics
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
----------------
jmolloy wrote:
> 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.
Thanks, I would prefer to defer floating point intrinsics for time being.

================
Comment at: docs/LangRef.rst:9586
@@ +9585,3 @@
+
+    These intrinsics are primarily used during optimization phase.
+
----------------
jmolloy wrote:
> 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?
In fact I meant this.
"These intrinsics are primarily used during code generation stage of compilation.They are generated by the compiler passes such as Loop/SLP vectorizer."

================
Comment at: docs/LangRef.rst:9619
@@ +9618,3 @@
+
+'``llvm.uhadd.*``' and '``llvm.shadd.*``'Intrinsic
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
----------------
jmolloy wrote:
> 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.
Even I was not sure about this. However, as ARM has both signed / unsigned horizontal add instruction, how would you decide to to emit the proper one?

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

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

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2888
@@ +2887,3 @@
+    }
+    Results.push_back(Tmp1);
+    break;
----------------
jmolloy wrote:
> What happens if the "if" condition fails? Tmp1 is a dangling pointer at the moment.
Will be handled in follow up patches.

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

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:160
@@ -155,1 +159,3 @@
 
+SDValue DAGTypeLegalizer::PromoteIntRes_AbsoluteDiff(SDNode *N) {
+  SDValue LHS, RHS;
----------------
jmolloy wrote:
> 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.
Will be handled in follow up patches.

================
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;
----------------
jmolloy wrote:
> Why are you not handling ISD::SABD and friends here?
Will be handled accordingly.

http://reviews.llvm.org/D10273

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






More information about the llvm-commits mailing list