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

[Cong Hou via llvm-commits]

congh marked an inline comment as done.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2377
@@ +2376,3 @@
+      if (Inst->getOpcode() == OpCode || isa<PHINode>(U)) {
+        UsersToVisit.push_back(U);
+      } else if (const ShuffleVectorInst *ShufInst =
----------------
hfinkel wrote:
> For PHIs, don't we need to check that the initial value is the operation identify value (0 for add, 1 for multiply, etc.)? And that there are only two unique incoming blocks?
I think this may not be necessary. We only care about how the values in the vector are used or how they flow to other defs. They can flow to phi nodes, but we need to check all uses of those phi nodes. Otherwise before reduction on elements, they can only flow to other defs of the same associative operation (e.g. add). As long as the reduction on elements is the only def where they are flowing to, it is safe to assume the operation is a vector-reduction one.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2410
@@ +2409,3 @@
+          return false;
+      } else if (isa<ExtractElementInst>(U)) {
+        // At this moment we should have reduced all elements in the vector.
----------------
hfinkel wrote:
> We should also catch the way that this is often programmed "by hand":
> 
>   typedef float v4f __attribute__((vector_size(16)));
>   v4f foo(void);
>   float bar() {
>     v4f phi = { 0, 0, 0, 0 };
>     for (int i = 0; i < 1600; ++i)
>       phi += foo();
> 
>     return phi[0] + phi[1] + phi[2] + phi[3];
>   }
> 
> thus, we have multiple extracts instead of shuffles. One might argue that we should canonicalize this to the shuffle form, but we don't have anything that does this currently (AFAIK). What do you think?
> 
OK, as long as this pattern often appears. After all, programs made by hand can have many different forms. For example, we can use target specific intrinsics (like movehl or hadd on X86) in the final reduction. As we don't have canonicalization now, I agree that we should make this function open to support more patterns.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2457
@@ -2341,2 +2456,3 @@
   Flags.setNoUnsignedWrap(nuw);
+  Flags.setVectorReduction(vec_redux);
   if (EnableFMFInDAG) {
----------------
hfinkel wrote:
> You need to set this flag on the PHI node too (meaning the associate CopyFromReg, etc. instructions), right?
Yes, we can do this, but I am wondering how we use the flag on PHI node during instruction combine (in my case I only check flags on operation nodes, but I think there may be other cases in which reduction PHI node can help?).

================
Comment at: test/CodeGen/Generic/vector-redux.ll:2
@@ +1,3 @@
+; RUN: llc < %s -debug-only=isel -o /dev/null 2>&1 | FileCheck %s
+
+ at a = global [1024 x i32] zeroinitializer, align 16
----------------
hfinkel wrote:
> You need to add:
> 
>   ; REQUIRES: asserts
> 
> here because you're checking debug output which is available only in +Asserts builds.
OK. Thanks for pointing it out!


http://reviews.llvm.org/D15250