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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

I was testing the code and ran into some problems with debug metadata on the loop nodes (actually using -Rpass=unroll in that case). Can you make sure that works as expected?



================
Comment at: docs/TransformMetadata.rst:110
+The followup attributes of a transformation that cannot be applied will
+never added to a loop and are therefore effectively ignored. This means
+that any followup-transformation in such attributes requires that its
----------------
Nit: never be added


================
Comment at: include/llvm/Transforms/Utils/LoopUtils.h:487
+
+/// Create a new loop identifier for a loop created from a loop transformations.
+///
----------------
Nit: transformations->transformation


================
Comment at: include/llvm/Transforms/Utils/LoopUtils.h:495
+///                                 are considered:
+///        nullptr   : Do not ihnerit any attribute from @p OrigLoopID; only use
+///                    those specified by a followup attribute.
----------------
Nit: inherit


================
Comment at: include/llvm/Transforms/Utils/LoopUtils.h:507
+///         None         : No followup attribute was found; it is up to the
+///                        transformation to chose attributes that make sense.
+///         @p OrigLoopID: The original identifier can be reused.
----------------
Nit: choose


================
Comment at: include/llvm/Transforms/Utils/LoopUtils.h:537
+  /// metadata, it must not be dropped. If the transformation could not be
+  /// applied, a warnign should be emitted.
+  TM_ForcedByUser = TM_Enable | TM_Force,
----------------
Nit: warning


================
Comment at: lib/Transforms/Scalar/LoopUnrollAndJamPass.cpp:194
 
+  TransformationMode EnableMode = hasUnrollAndJamTransformation(L);
+  if (EnableMode & TM_Disable) {
----------------
This code will need rebasing. There is a check earlier that looks for disable metadata that could be replaced by this. Look for HasUnrollDisablePragma/HasUnrollAndJamDisablePragma. If the same was done for unrolling, I think that would remove the need for the IgnoreUser (although your comment about it is probably still true).


================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:1384
+  bool InheritAllAttrs = !InheritOptionsExceptPrefix;
+  bool InheritSomeAttrs =
+      InheritOptionsExceptPrefix && InheritOptionsExceptPrefix[0] != '\0';
----------------
Maybe InheritSomeAttrs -> InheritNonExceptAttrs?


Repository:
  rL LLVM

https://reviews.llvm.org/D49281