[PATCH] D59832: [LoopRotation] Allow loop header duplication if vectorization is forced

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:60
 
+static bool isForcedVectorize(Loop *L) {
+  Optional<const MDOperand *> Value =
----------------
Meinersbur wrote:
> anemet wrote:
> > Meinersbur wrote:
> > > anemet wrote:
> > > > Meinersbur wrote:
> > > > > Consider using `hasVectorizeTransformation` in LoopUtils.h. I added it to make determining whether a pragma is added consistent between passes.
> > > > Nice, I didn't know about this.  BTW, LoopDistribute was not updated to use hasDistributeTransformation.
> > > I only changed UnrollAndJam to use it for now. How these decisions are made is surprisingly subtle and e.g. in case of LoopVectorizeHints, integrated within other decisions. I was not brave enough to put up reviews for this and risking changing behavior without a need. But I would be happy if new code uses it.
> > @Meinersbur  I am actually confused whether I should be checking for Enable or ForcedByUser.  We only set Enable and not Forced if the vectorization width is requested via a pragma but I think that is an even stronger enforcement of vectorization.  What is the logic here? 
> `ForcedByUser` follows `llvm.loop.vectorize.enable`, i.e. whether `#pragma clang loop vectorize(enable)` is present. Other metadata such as `llvm.loop.vectorize.width` logically are only used in case the vectorizer decides to transform a loop, without explicitly enabling it. Other passes like LoopUnroll and LoopUnrollAndJam enable themselves even if only `llvm.loop.unroll.count` is present.
> 
> `LoopVectorizationHints::getForce()` only checks the `llvm.loop.vectorize.enable` setting to override heuristicts, so I suggest to use `ForcedByUser`.
> 
> This is what I mean by "subtle".
That's an excellent point. I think after D66290 Clang also generates `vectorize.enable` when just the `width` pragma is specified, so for code generated by clang, this issue should not exist now, but may for other frontends.


================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:119
+        SQ, false,
+        isForcedVectorize(L) ? DefaultRotationThreshold : MaxHeaderSize, false);
   }
----------------
fhahn wrote:
> Might be worth a comment why isForcedVectorize overrides the behavior here.
I added a variable similar to what we have in the new pm version in the committed patch,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59832