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

[Shahid, Asghar-ahmad]

Hi James,

Thanks for your comments.

I am on vacation now. I will look into it accordingly afterwards.

Regards,
Shahid

> -----Original Message-----
> From: James Molloy [mailto:james.molloy at arm.com]
> Sent: Monday, June 08, 2015 1:34 PM
> To: Shahid, Asghar-ahmad; hfinkel at anl.gov; renato.golin at linaro.org;
> james.molloy at arm.com
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [PATCH] Add intrinsics and SDNodes for signed/unsigned
> absolute difference (absdiff) and horizontal add (hadd).
> 
> 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/
>