[PATCH] D75069: [LoopVectorizer] Inloop vector reductions

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:799
 
+static bool findPathToPhi(Instruction *Cur,
+                          SmallVectorImpl<Instruction *> &ReductionOperations,
----------------
nit: would be good to document what this function does


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:802
+                          unsigned Opcode, PHINode *Phi, Loop *L) {
+  assert(Cur->getOpcode() == Opcode);
+  assert(Cur->getNumOperands() == 2);
----------------
nit: would be good to add a message


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:786
+                              VPSlotTracker &SlotTracker) const {
+  O << " +\n"
+    << Indent << "\""
----------------
The ::print methods do not need to emit the " +\n" at the start and the "\\l\" parts after 4c8285c750bc


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:43
 #include "llvm/IR/IRBuilder.h"
+#include "llvm/Transforms/Utils/LoopUtils.h"
 #include <algorithm>
----------------
Would a forward decl of ReductionDesciptor be sufficient or is there another need to include LoopUtils.h?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1026
+  RecurrenceDescriptor *RdxDesc;
+  Instruction *I;
+  VPValue *ChainOp;
----------------
would be good to document the members.


================
Comment at: llvm/test/Transforms/LoopVectorize/reduction-inloop.ll:6
 
 define i32 @reduction_sum(i32 %n, i32* noalias nocapture %A, i32* noalias nocapture %B) nounwind uwtable readonly noinline ssp {
 ; CHECK-LABEL: @reduction_sum(
----------------
I think it would be also good to add a few additional simpler test cases, e.g. reductions with only 1 or 2 reduction steps in the loop, some with constant scalar operands. And with some of the (unnecessary trounces stripped). Also the test diff can be slightly reduced by having constant trip counts. 

It's good to have the more complex ones, but it would also be good to start with simpler cases. That way it is probably easier to follow what's going on. And it would reduce the diff quite a bit if the tests for the various different opcodes would be a bit more compact.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75069/new/

https://reviews.llvm.org/D75069