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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:60
 
+static bool isForcedVectorize(Loop *L) {
+  Optional<const MDOperand *> Value =
----------------
fhahn wrote:
> 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.
Yes, some consistency would be nice. IMHO that `vectorize_width` implicitly forces vectorization is a front-end interpretation. `vectorize.width` without `vectorize.enable` has a valid interpretation "if you are vectorizing, use this SIMD width".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59832