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

[Kerry McLaughlin via Phabricator via llvm-commits]

kmclaughlin 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());
   }
----------------
sdesmalen wrote:
> david-arm wrote:
> > sdesmalen wrote:
> > > is this change necessary?
> > It's fixing a missing case where we weren't previously allowing reordering for scalable VF=vscale x 1. I think it's worth fixing, but maybe it doesn't have to live in this patch?
> @kmclaughlin can this be pulled out into a separate patch, or does it depend on changes in this patch in order to test it?
> 
> I find the way the condition is written very confusing. It looks like the condition is synonymous, but it isn't. How about writing `EC.isVector()` instead?
@sdesmalen this doesn't depend on any other changes here so I've removed it from this patch


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:865
+
+  if (!EnableStrictReductions || Hints.getWidth().isScalar())
+    return false;
----------------
sdesmalen wrote:
> 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"`?
I've removed this check as I don't think it's necessary, but it was added to be consistent with allowReordering() which returns false if `EC.getKnownMinValue() > 1`


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

https://reviews.llvm.org/D101836