[PATCH] D15250: Detecte vector reduction operations just before instruction selection.

[Hal Finkel via llvm-commits]

hfinkel added inline comments.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2304
@@ +2303,3 @@
+/// Checks if the given instruction performs a vector reduction, in which case
+/// we have the freedom to reorganize the elements in the result as long as the
+/// reduction of them stay unchanged.
----------------
'reorganize' sounds a bit weak here. How about reorganize -> alter

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2305
@@ +2304,3 @@
+/// we have the freedom to reorganize the elements in the result as long as the
+/// reduction of them stay unchanged.
+static bool isVectorReductionOp(const User *I) {
----------------
stay -> stays

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2336
@@ +2335,3 @@
+  //
+  //   1. The reduction operation of the same arithmetic as the given
+  //      instruction.
----------------
'same arithmetic' sounds odd. Maybe just say 'same opcode'.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2341
@@ +2340,3 @@
+  //
+  //   3. ShuffleVector instruction together with a reduction operation that do
+  //      partial reduction.
----------------
do -> does a

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2347
@@ +2346,3 @@
+  //
+  // 3 & 4 above perform reduction on all elements of the vector. We push defs
+  // from 1-3 to the stack to continue the DFS. The given instruction is not
----------------
reduction -> a reduction

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2349
@@ +2348,3 @@
+  // from 1-3 to the stack to continue the DFS. The given instruction is not
+  // supposed to be a reduction operation if we meet any other insturctions
+  // other than those listed above.
----------------
insturctions (typo)

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2349
@@ +2348,3 @@
+  // from 1-3 to the stack to continue the DFS. The given instruction is not
+  // supposed to be a reduction operation if we meet any other insturctions
+  // other than those listed above.
----------------
hfinkel wrote:
> insturctions (typo)
Remove 'supposed to be'

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2367
@@ +2366,3 @@
+
+      if (Inst->getOpcode() == OpCode || isa<PHINode>(U)) {
+        UsersToVisit.push_back(U);
----------------
We also need to check the fast-math flags on fadd/fmul here.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2367
@@ +2366,3 @@
+
+      if (Inst->getOpcode() == OpCode || isa<PHINode>(U)) {
+        UsersToVisit.push_back(U);
----------------
hfinkel wrote:
> We also need to check the fast-math flags on fadd/fmul here.
We can allow selects here for the same reason we can allow phis, right? If so, we should.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2375
@@ +2374,3 @@
+        // ElemNumToReduce / 2 elements in another vector.
+
+        if (!isa<UndefValue>(U->getOperand(1)))
----------------
Do we need to check that we don't have ElemNumToReduce == 1 here?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2398
@@ +2397,3 @@
+          UsersToVisit.push_back(U2);
+          ElemNumToReduce /= 2;
+        } else
----------------
When  ElemNumToReduce == 1 here, do we need to check that the only user is an ExtractElementInst? I'm concerned that it could be a another reduction operation before the extract.


http://reviews.llvm.org/D15250