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

[Adam Nemet via Phabricator via llvm-commits]

anemet marked 2 inline comments as done.
anemet added a comment.

In D59832#1444555 <https://reviews.llvm.org/D59832#1444555>, @Meinersbur wrote:

> This might be a more general problem, other passes might expect a normalized form as well, such as UnrollAndJam and LoopDistribute.
>
> For LoopVectorizer, I am somewhat surprised. It calls simplifyLoop itself (instead of relying on LoopSimplifyPass). Could the same be done for LoopRotation?


It felt that the decision belonged to LoopRotate.  We make a profitability decision there (for -Oz loop rotate is a bad trade-off) which is inaccurate.

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



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


================
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
----------------
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.






Repository:
  rL LLVM

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

https://reviews.llvm.org/D59832