[PATCH] D113973: [LoopVectorize][CostModel] Choose smaller VFs for in-loop reductions with no loads/stores

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for the latest update @RosieSumpter. The patch seems to improve the case based on the types used in the reductions now, so that seems like an improvement. Just left a few final nits.



================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:481
 
+  SmallPtrSet<Instruction *, 4> CastsToRecurTy;
+
----------------
nit: move to line 274 to be together with `CastsFromRecurTy` ?


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:518
+  // recurrence type is equal to the recurrence type, the recurrence expression
+  // will be represented in a narrower type. If there are any cast instructions
+  // that will be unnecessary, collect them in CastsFromRecurTy. Note that the
----------------
nit: narrower or wider


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:522
+  //
+  // TODO: A better way to represent this may be to tag in some way all the
+  //       instructions that are a part of the reduction. The vectorizer cost
----------------
nit: This is unrelated to your patch, but to me it looks like this mechanism is implemented the wrong way around. I would have expected it to unconditionally capture all cast instructions (to/from recurrence type), and in LoopVectorize to call `computeRecurrenceType`, and make decisions to ignore cast instructions based on the narrower recurrence type.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5989-5990
+          MaxWidth,
+          DL.getTypeSizeInBits(RdxDesc.getRecurrenceType()->getScalarType())
+              .getFixedSize());
+      // We also need to check the cast instructions in the loop as there may be
----------------
nit: this can be `RdxDesc.getCurrenceType()->getScalarSizeInBits()`


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5996-5997
+            MaxWidth,
+            DL.getTypeSizeInBits(cast<CastInst>(I)->getSrcTy()->getScalarType())
+                .getFixedSize());
+      }
----------------
nit: same here: `cast<CastInst>(I)->getSrcTy()->getScalarSizeInBits()`


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

https://reviews.llvm.org/D113973