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

[James Molloy]

jmolloy requested changes to this revision.

This revision now requires changes to proceed.

Hi,

Generally looks good, but I have some initial comments.

Cheers,

James


================
Comment at: docs/LangRef.rst:9593
@@ +9592,3 @@
+      declare <4 x integer> @llvm.hadd.v4i32(<4 x integer> %a)
+      declare <4 x float> @llvm.hadd.v4f32(<4 x float> %a)
+
----------------
You need to be very explicit about the behaviour of this intrinsic with floating point arguments. What order, if any, does it perform the adds in? If there is no guaranteed order, it can only be used in fast-math mode.

================
Comment at: include/llvm/IR/Intrinsics.td:599
@@ +598,3 @@
+// Calculate the horizontal/reduction sum across the elements of input vector.
+def int_hadd_int : Intrinsic<[llvm_anyint_ty], [llvm_anyvector_ty], [IntrNoMem]>;
+def int_hadd_float : Intrinsic<[llvm_anyfloat_ty], [llvm_anyvector_ty], [IntrNoMem]>;
----------------
Just having one intrinsic here would be good; there's no need for a separate int and float version.

================
Comment at: include/llvm/IR/Intrinsics.td:601
@@ -598,2 +600,3 @@
+def int_hadd_float : Intrinsic<[llvm_anyfloat_ty], [llvm_anyvector_ty], [IntrNoMem]>;
 //===-------------------------- Masked Intrinsics -------------------------===//
 //
----------------
Blank line missing here.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:712
@@ -707,1 +711,3 @@
+  case ISD::FHADD:
+    return UnrollHADD(Op);
   default:
----------------
Can't you just call ExpandHADD() here? or at least share the unroll and expand code?


Repository:
  rL LLVM

http://reviews.llvm.org/D10964