[PATCH] D10964: [Codegen] Add intrinsics 'hadd*' and corresponding SDNodes for horizontal sum operation.

[Bruno Cardoso Lopes]

bruno added a comment.

Hi,

Thanks for working on this. Comments below.


================
Comment at: docs/LangRef.rst:9620
@@ +9619,3 @@
+    call i32 @llvm.hadd.v4i32(<4 x i32> %a)
+
+is equivalent to::
----------------
I don't know if this discussion already happend, but I've been thinking about this and I'm wondering whether we should have a vector result instead of a scalar one; the result in the first element of the vector type and the other elements undef. Then an extractelement follows to get the scalar result.

IMO, this is more natural given the way architectures implement variants of HADD, they usually leave the results on vectors. One advantage of doing this is that we can also use this ISD::HADD while lowering other vector operations (the CTPOP case) and don't have to write a DAGCombine or any other extra logic to recognise the vector back from an extract. I might be biased on one side of the history here though, I'd appreciate hearing the other side :-)

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2876
@@ +2875,3 @@
+  case ISD::HADD: {
+    SDValue OpVal = Node->getOperand(0);
+    EVT VT = OpVal.getValueType();
----------------
Note: If we use vector result instead of scalar, this won't be needed here and everything could be handled in LegalizeVectorOps.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:1048
@@ -1013,1 +1047,3 @@
+  return Ops;
+}
 }
----------------
For the integer part (ISD::HADD) I believe you could do "vector shifts + vector adds" instead of "extracts + scalar adds", probably better to do not leave the vector domain? In case the current target doesn't support "vector shifts + vector adds" for the element type, then your implementation should fallback to "extracts + scalar adds". To check that you can use in UnrollHADD:

  if (TLI.getOperationAction(ISD::SHL, VT) == TargetLowering::Expand ||
      TLI.getOperationAction(ISD::ADD, VT) == TargetLowering::Expand)
   ....



Repository:
  rL LLVM

http://reviews.llvm.org/D10964