[PATCH] D12202: Add Support for Small Size Reductions

[James Molloy via llvm-commits]

jmolloy requested changes to this revision.
jmolloy added a comment.
This revision now requires changes to proceed.

Hi,

You've convinced me that our two patches can actually sit side by side. Please see my specific comments in the patch.

Cheers,

James


================
Comment at: include/llvm/Transforms/Utils/LoopUtils.h:197
@@ +196,3 @@
+  /// Returns true if the recurrence kind is an arithmetic kind.
+  static bool isArithmeticRecurrenceKind(RecurrenceKind Kind);
+
----------------
What would a non-arithmetic kind be? I'm not sure I understand this.

================
Comment at: include/llvm/Transforms/Utils/LoopUtils.h:203
@@ +202,3 @@
+  /// operation. The AND operation is also added to CI.
+  static Instruction *lookThroughAnd(PHINode *Phi, Type *&RT,
+                                     SmallPtrSetImpl<Instruction *> &Visited,
----------------
I don't like this interface. But that shouldn't stop this patch I think - I think we need to have a good hard redesign of the interface for RecurrenceDescriptor (and InductionDescriptor which I'm ripping out of LoopVectorize in D12284.

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:270
@@ -133,3 +269,3 @@
     // Check  whether we found a reduction operator.
-    FoundReduxOp |= !IsAPhi;
+    FoundReduxOp |= (!IsAPhi && (Cur != Start));
 
----------------
I think you'vr got two sets of parens too many here.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3332
@@ +3331,3 @@
+    // smaller type.
+    if ((VF > 1) && (RdxPhi->getType() != RdxDesc.getRecurrenceType())) {
+      Type *RdxVecTy = VectorType::get(RdxDesc.getRecurrenceType(), VF);
----------------
You're got too many parens here.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3336
@@ +3335,3 @@
+      for (unsigned part = 0; part < UF; ++part) {
+        Value *Trunc = Builder.CreateTrunc(RdxExitVal[part], RdxVecTy);
+        Value *Extnd = RdxDesc.isSigned()
----------------
Wouldn't it be just as easy to AND with Type->getMask()?


http://reviews.llvm.org/D12202