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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a reviewer: Meinersbur.
fhahn added a comment.

This makes sense to me, given that loops with bottom checks are a precondition for LV. I think it would be good to also update the section about llvm.loop.vectorize.enable in LangRef to say it might also enable transformations like LoopRotate, in preparation for LV.

In case LV fails to vectorize the loop, it might be a bit surprising the loop has been rotated. But the way things are structured at the moment, there's nothing we can do about that (we cannot tell in LoopRotate if LV will be able to vectorize). By documenting that behavior, we can push the responsibility for that to the user. Maybe a remark in LoopRotate would be helpful to indicate that we only rotated because of the metadata.

Also adding Michael, as he worked on loop related metadata recently.



================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:119
+        SQ, false,
+        isForcedVectorize(L) ? DefaultRotationThreshold : MaxHeaderSize, false);
   }
----------------
Might be worth a comment why isForcedVectorize overrides the behavior here.


================
Comment at: test/Transforms/LoopVectorize/AArch64/Oz-and-forced-vectorize.ll:1
+; RUN: opt -Oz -S < %s | FileCheck %s
+
----------------
Could you limit the test case to running loop-rotate? The tests for LV should already be sufficient to check the transformed case. (same for second test)


================
Comment at: test/Transforms/LoopVectorize/AArch64/Oz-and-forced-vectorize.ll:59
+!24 = !DILocation(line: 16, column: 9, scope: !7)
+!25 = !{!"llvm.loop.vectorize.enable", i1 true}
----------------
All metadata except this one can be stripped I think.  (same for second test)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59832