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

Wed Mar 27 08:01:53 PDT 2019

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

In D59832#1444244 <https://reviews.llvm.org/D59832#1444244>, @fhahn wrote:

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

I think the picture is even simpler in this case.  The user has asked for an optimization.  If we can't deliver we issue a warning (-Wpass-failed).  So the user has sufficient information to see that something went wrong.  But yes a loop rotation remark makes sense! especially mentioning that we did it in preparation for forced vectorization.

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

================
Comment at: test/Transforms/LoopVectorize/AArch64/Oz-and-forced-vectorize.ll:1
+; RUN: opt -Oz -S < %s | FileCheck %s
+
----------------
fhahn wrote:
> 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)
I don't think so.  There is no other way to get sizelevel=2 activated for opt.  You either run the entire -Oz pipeline or you run specific tests with sizelevel=0.

================
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}
----------------
fhahn wrote:
> All metadata except this one can be stripped I think.  (same for second test)
I kept it in to get the *warning* with proper line info for failed user-requested optimization.  I can drop if you don't think that is useful.  I liked it because it completely captured the user experience that this test came from.

I can check for the actual diagnostics to make this less subtle. 

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59832