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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

In D59832#1444678 <https://reviews.llvm.org/D59832#1444678>, @anemet wrote:

> If you feel strongly about this I can explore rotating the loop in the vectorizer when I have a bit more time.


No, this would go beyond fixing a bug.



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


================
Comment at: test/Transforms/LoopVectorize/AArch64/Oz-and-unforced-vectorize.ll:27
+  %mul3 = fmul float %0, %1, !dbg !21
+; CHECK-NOT: fmul <4 x float>
+  store float %mul3, float* %arrayidx2, align 4, !dbg !21, !tbaa !16
----------------
anemet wrote:
> Meinersbur wrote:
> > I don't think we need to regression-test that we do not vectorize. If LoopVectorize one day manages to vectorize unrotated loop, it's a good thing, not a regression.
> > 
> > Btw, since it's the same content, you could instead add another `RUN:` line to the other test.
> > I don't think we need to regression-test that we do not vectorize. If LoopVectorize one day manages to vectorize unrotated loop, it's a good thing, not a regression.
> 
> I like to have the negative tests as well but if you want I'll remove it.
>  
> > Btw, since it's the same content, you could instead add another RUN: line to the other test.
> 
> Well, it's not the same content since it's missing the the forced-vectorize metadata.  But perhaps I am not understanding the point you're trying to make.
> 
> 
> 
> 
> I like to have the negative tests as well but if you want I'll remove it.

I don't like them; they are fragile, require maintenance and check-llvm takes longer.

> Well, it's not the same content since it's missing the the forced-vectorize metadata.

I was assuming the missing `llvm.loop.vectorize.enable` metadata was a mistake. Without it, LoopVectorize won't do anything, even if it could vectorize the loop (There is a difference in how clang and opt initialize the PassManagerBuilder).

An excellent example of how fragile this kind of test is.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59832