[PATCH] D57728: Relax constraints for reduction vectorization

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for these changes to your patch @sanjoy! The patch is looking in good shape, just a few remaining nits mostly.



================
Comment at: include/llvm/Analysis/IVDescriptors.h:244
   RecurrenceKind Kind = RK_NoRecurrence;
+  // The fast-math flags on the recurrenct instructions.  We propagate these
+  // fast-math flags into the vectorized FP instructions we generate.
----------------
nit: recurrenct -> recurrent


================
Comment at: lib/Analysis/IVDescriptors.cpp:254
 
+  FastMathFlags FMF;
+  FMF.setFast();
----------------
Is it worth adding a comment describing that FMF will be an intersection of the FastMathFlags from all the reduction operations (and thus needs to start with the full set of flags)?


================
Comment at: lib/Analysis/IVDescriptors.cpp:302
         return false;
+      if (isa<FPMathOperator>(ReduxDesc.getPatternInst())) {
+        FMF &= ReduxDesc.getPatternInst()->getFastMathFlags();
----------------
nit: unnecessary curly braces


================
Comment at: lib/Analysis/IVDescriptors.cpp:563
   Instruction *UAI = Prev.getUnsafeAlgebraInst();
-  if (!UAI && isa<FPMathOperator>(I) && !I->isFast())
+  if (!UAI && !CanVectorizeReduction(I))
     UAI = I; // Found an unsafe (unvectorizable) algebra instruction.
----------------
nit: since 'CanVectorizeReduction()' is only used once, it probably makes more sense to just expand it here and remove the function.


================
Comment at: lib/CodeGen/ExpandReductions.cpp:124
+                  : getShuffleReduction(Builder, Vec, getOpcode(ID), MRK,
+                                        FastMathFlags::getFast());
     II->replaceAllUsesWith(Rdx);
----------------
Rather than choosing 'getFast() and going through 'IsOrdered' (which is set when the full 'fast' property is set), do we want to take the FastMathFlags from the IntrinsicInst directly?


================
Comment at: lib/CodeGen/ExpandReductions.cpp:124
+                  : getShuffleReduction(Builder, Vec, getOpcode(ID), MRK,
+                                        FastMathFlags::getFast());
     II->replaceAllUsesWith(Rdx);
----------------
sdesmalen wrote:
> Rather than choosing 'getFast() and going through 'IsOrdered' (which is set when the full 'fast' property is set), do we want to take the FastMathFlags from the IntrinsicInst directly?
Note that there is a test missing for this change.


================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:675
+static Value *addFastMathFlag(Value *V, FastMathFlags FMF) {
   if (isa<FPMathOperator>(V)) {
+    cast<Instruction>(V)->setFastMathFlags(FMF);
----------------
nit: unnecessary curly braces.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:322
 static Value *addFastMathFlag(Value *V) {
   if (isa<FPMathOperator>(V)) {
+    cast<Instruction>(V)->setFastMathFlags(FastMathFlags::getFast());
----------------
nit: unnecessary curly braces.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:329
+static Value *addFastMathFlag(Value *V, FastMathFlags FMF) {
+  if (isa<FPMathOperator>(V)) {
+    cast<Instruction>(V)->setFastMathFlags(FMF);
----------------
nit: unnecessary curly braces.


================
Comment at: test/Transforms/LoopVectorize/reduction-fastmath.ll:48
+; CHECK: %wide.load = load <4 x float>, <4 x float>*
+; CHECK: ret float %sum.lcssa
+  ret float %sum.lcssa
----------------
Do you want to add a check for the 'fast' attribute on the reductions in the middle.block, similar to what you've done for reduction_sum_float_only_reassoc_and_contract ?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57728