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

[Michael Kruse via Phabricator via llvm-commits]

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


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:761
 
+  if (!IgnoreUser) {
+    TransformationMode EnableMode = hasUnrollTransformation(L);
----------------
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.


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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D49281