[PATCH] D49281: [Unroll/UnrollAndJam/Vectorizer/Distribute] Add followup loop attributes.

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:761
 
+  if (!IgnoreUser) {
+    TransformationMode EnableMode = hasUnrollTransformation(L);
----------------
Meinersbur wrote:
> dmgreen wrote:
> > This shouldn't be needed here. Before this patch, there was a single place that checked if the loop had unroll disable pragma (HasUnrollDisablePragma at the start of tryToUnrollLoop). It seems best to keep that as-is in this patch (it's already long enough!) and remove HasUnrollDisablePragma, replacing it with the new hasUnrollTransformation & TM_Disable check. Then we won't need this IgnoreUser.
> This is here because if the unfortunate interaction between LoopUnroll and LoopUnrollAndJam. `computeUnrollAndJamCount` uses the result of this function to itself determine whether it should unroll-and-jam. 
> 
> `HasUnrollDisablePragma` checks for the `llvm.loop.unroll.enable` property. `hasUnrollTransformation` returns whether LoopUnroll should do something which is not interchangeable. For some reason, `llvm.loop.unroll.enable` is handled here, but `llvm.loop.unroll.count` and `llvm.loop.unroll.full` are handled here and therefore have in influence on LoopUnrollAndJam.
> 
> I would be glad if you, the author of LoopUnrollAndJam, could untangle this.
Sometimes it's easier to show with code :-) so this is what I was thinking of:
https://reviews.llvm.org/P8121
Unless you think that will not work for some reason? It passes all the tests you have here, and removes HasUnrollDisablePragma and the IgnoreUser, so seems cleaner. It also has the advantage of keeping unrelated changes to a minimum and not introducing a second place for llvm.loop.unroll.disable to be checked.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D49281