[PATCH] D101836: [LoopVectorize] Enable strict reductions when allowReordering() returns false

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h:146
     return getForce() == LoopVectorizeHints::FK_Enabled ||
-           EC.getKnownMinValue() > 1;
+           (EC.isNonZero() && !EC.isScalar());
   }
----------------
is this change necessary?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:865
+
+  if (!EnableStrictReductions || Hints.getWidth().isScalar())
+    return false;
----------------
sdesmalen wrote:
> This will also return false if there are no reductions at all, or if all reductions are unordered?
Should a hint of VF=1 really lead to the diagnostic `"loop not vectorized: cannot prove it is safe to reorder floating-point operations"`?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:865-866
+
+  if (!EnableStrictReductions || Hints.getWidth().isScalar())
+    return false;
+
----------------
This will also return false if there are no reductions at all, or if all reductions are unordered?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:868-871
+  return (all_of(getReductionVars(), [&](auto &Reduction) -> bool {
+    RecurrenceDescriptor RdxDesc = Reduction.second;
+    return !RdxDesc.hasExactFPMath() || RdxDesc.isOrdered();
+  }));
----------------
How about returning true if for each reduction variable, any of the following conditions is true:
  1. The reduction is no ExactFPMath instruction for the reduction.
  2. The reduction is unordered.
  3. EnableStrictReductions is true.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:872
+  }));
+}
+
----------------
I'd prefer the default case to be `return false;`, i.e. when we cannot explicitly determine it is safe, we assume it isn't safe. That would handle the case where `Requirements->getExactFPInst()` is true, but it isn't an instruction used in the reduction. (although I don't know if that would ever happen?)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9857
 
-  if (!Requirements.canVectorizeFPMath(Hints)) {
+  if (!LVL.canVectorizeFPMath(Hints, EnableStrictReductions)) {
     ORE->emit([&]() {
----------------
You don't need to pass EnableStrictReductions, since it is defined in the same file?


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

https://reviews.llvm.org/D101836