[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/Utils/LoopUtils.cpp:297
+      if (InheritSomeAttrs) {
+        auto AttrName = cast<MDString>(Op->getOperand(0).get())->getString();
+        if (AttrName.startswith(InheritOptionsExceptPrefix)) {
----------------
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D49281