[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());
   }
----------------
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?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9857
 
-  if (!Requirements.canVectorizeFPMath(Hints)) {
+  if (!LVL.canVectorizeFPMath(Hints, EnableStrictReductions)) {
     ORE->emit([&]() {
----------------
david-arm wrote:
> sdesmalen wrote:
> > You don't need to pass EnableStrictReductions, since it is defined in the same file?
> I think it has to be passed since `EnableStrictReductions` lives in LoopVectorize.cpp and canVectorizeFPMath lives in LoopVectorizationLegality.cpp.
You're right, I didn't realise that, thanks!


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

https://reviews.llvm.org/D101836