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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur marked 2 inline comments as done.
Meinersbur added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:761
 
+  if (!IgnoreUser) {
+    TransformationMode EnableMode = hasUnrollTransformation(L);
----------------
dmgreen wrote:
> 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.
Thank you for the patch. I am not 100% sure whether this does not change LoopUnroll's behavior. That is, with `!{!"llvm.loop.unroll.count", i32 1}` it currently executes
```
    UP.Count = PragmaCount;
    UP.Runtime = true;
    UP.AllowExpensiveTripCount = true;
    UP.Force = true;
    if ((UP.AllowRemainder || (TripMultiple % PragmaCount == 0)) &&
        getUnrolledLoopSize(LoopSize, UP) < PragmaUnrollThreshold)
      return true;
```
where as with your patch it bails out early (it might still do peeling even if UP.Count is 1). Also, the `-unroll-count` command-line option would be evaluated first before your patch. {F7643957} fails with your patch.

However, I like that it indeed makes the unroll decision simpler and goes in the direction of separating LoopUnroll and LoopUnrollAndJam's decision logic.


================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:297
+      if (InheritSomeAttrs) {
+        auto AttrName = cast<MDString>(Op->getOperand(0).get())->getString();
+        if (AttrName.startswith(InheritOptionsExceptPrefix)) {
----------------
dmgreen wrote:
> Meinersbur wrote:
> > dmgreen wrote:
> > > Would this fall over if the metadata was not a string? Such as debug metadata.
> > This was previously checked to be in a LoopID, therefore cannot be debug metadata.
> > 
> > This assumes that the metadata is not malformed. However, this is nowhere handled gracefully in LLVM. For instance, `UnrollAndJamCountPragmaValue` will trigger an assertion if the MDNode has not exactly 2 items, or the second item is something else than a positive integer. In the case here, an assertion in `cast<T>` will trigger.
> > 
> > I added extra checks at this location, but there are many others.
> Yeah, malformed input would be fine to not handle, as far as I understand (or perhaps is just QOI). But I was testing something like this (hope I still have it correct):
> ```
> void c(int n, int* w, int* x, int *y, int* z, int *a) {
> #pragma clang loop distribute(enable)  vectorize(disable)
>     for (int i=0; i < n; i++) {
>         x[i] = y[i] + z[i]*w[i];
>         a[i+1] = (a[i-1] + a[i] + a[i+1])/3.0;
>         y[i] = z[i] - x[i];
>     }
> }
> ```
> Ran with "clang  -O3 distribute.c -S -g" would crash with the previous patch. Now I think it doesn't drop the distribute metadata? I believe the llvm.loop metedata will looks something like !58 in:
> ```
> !58 = distinct !{!58, !30, !59, !60, !61, !62}
> !59 = !DILocation(line: 8, column: 5, scope: !20)
> !60 = !{!"llvm.loop.vectorize.width", i32 1}
> !61 = !{!"llvm.loop.unroll.disable"}
> !62 = !{!"llvm.loop.distribute.enable", i1 true}
> ```
> !30 is a DILocation too, which I think are the parts causing the problems.
I may not have considered that CGLoopInfo.cpp also adds debug locations to LoopIDs. Should be fixed with the previous update. Thanks for noticing.

I also made a mistake in that update which dropped all non-distribute metadata instead of the distribute metadata. It made one regression test fail.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D49281