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

[hfinkel@anl.gov via llvm-commits]

hfinkel added inline comments.

================
Comment at: docs/LangRef.rst:10823
@@ +10822,3 @@
+"""""""
+This is an overloaded intrinsic. The loaded data is a vector of any integer or floating point data type.
+Order of additions performed by the intrinsic is undefined.
----------------
Nothing is being loaded here. You can just say that, "The argument is a vector of any integer or floating-point type."


================
Comment at: docs/LangRef.rst:10836
@@ +10835,3 @@
+The ``llvm.hsum`` intrinsic returns the result of the horizontal or reduction sum of the elements of the
+vector operand, treating it as integers or floats. If the result overflows, the behavior is undefined.
+
----------------
Signed or unsigned overflow?


================
Comment at: docs/LangRef.rst:10841
@@ +10840,3 @@
+    These intrinsics are primarily used during the code generation stage of
+    compilation.They are generated by the compiler passes such as the Loop and
+    SLP vectorizers.
----------------
Missing space before "They"

================
Comment at: docs/LangRef.rst:10843
@@ +10842,3 @@
+    SLP vectorizers.
+    The expectation is that, the frontends should not need to generate these
+    intrinsics themselves.
----------------
No need for a comma after that.


================
Comment at: docs/LangRef.rst:10849
@@ +10848,3 @@
+
+The argument is vector of integer or floating point number.
+
----------------
integer or floating point number -> integer or floating-point type.

================
Comment at: docs/LangRef.rst:10858
@@ +10857,3 @@
+
+is equivalent to::
+
----------------
I think this is unhelpful. Given that the order of additions is undefined, it might not be exactly equivalent to this code. I think describing this in words would be better in this case.



================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:1069
@@ +1068,3 @@
+        DAG.getVectorShuffle(VT, dl, OpVal, DAG.getUNDEF(VT), &ShuffleMask[0]);
+    OpVal = DAG.getNode(Op->getOpcode() == ISD::HSUM ? ISD::ADD : ISD::FADD, dl,
+                        VT, OpVal, Shuffle);
----------------
Given that the order of additions is undefined, we can add NoSignedWrap or NoUnsignedWrap in the integer case.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:711
@@ +710,3 @@
+  Hi = DAG.getNode(N->getOpcode(), dl, NewVT, Hi);
+  Lo = DAG.getNode(N->getOpcode() == ISD::HSUM ? ISD::ADD : ISD::FADD, dl,
+                   NewVT, Lo, Hi);
----------------
Same comment here as above (we can add signed or unsigned nowrap here).


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:2941
@@ +2940,3 @@
+  SDValue Ops;
+  Ops = DAG.getNode(N->getOpcode() == ISD::HSUM ? ISD::ADD : ISD::FADD, dl,
+                    EltVT, LHSElem, RHSElem);
----------------
Same here.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:2946
@@ +2945,3 @@
+                          DAG.getConstant(i, dl, TLI.getVectorIdxTy(DAG.getDataLayout())));
+    Ops = DAG.getNode(N->getOpcode() == ISD::HSUM ? ISD::ADD : ISD::FADD, dl,
+                      EltVT, LHSElem, Ops);
----------------
And here.


http://reviews.llvm.org/D10964