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

[hfinkel@anl.gov via llvm-commits]

hfinkel added inline comments.

================
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 =
----------------
congh wrote:
> 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.
Sounds good.

================
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.
----------------
congh wrote:
> 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.
Thinking about it, I think it is better to canonicalize these in instcombine. The shuffle form will use fewer instructions in all cases (except the two-element case). Don't bother matching it here, we should add canonicalization to instcombine in a separate patch.

FWIW, however, yes, I've seen code like this from several users at our facility.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2457
@@ -2341,2 +2456,3 @@
   Flags.setNoUnsignedWrap(nuw);
+  Flags.setVectorReduction(vec_redux);
   if (EnableFMFInDAG) {
----------------
congh wrote:
> 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?).
Okay; don't bother then. I was thinking you needed it for isel, but if not, wait until we have a concrete use case.


http://reviews.llvm.org/D15250