[PATCH] D49281: [Unroll/UnrollAndJam/Vectorizer/Distribute] Add followup loop attributes.
Dave Green via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 14 04:35:47 PDT 2018
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
More information about the llvm-commits
mailing list