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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

LGTM.

There is an inline comment on the previous diff that I think was never submitted; It does not apply to the latest diff as it already uses `TM_ForcedByUser`. I don't delete is since is might still be interesting.



================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:60
 
+static bool isForcedVectorize(Loop *L) {
+  Optional<const MDOperand *> Value =
----------------
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".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59832