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

Dave Green via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun Jul 15 13:15:04 PDT 2018


dmgreen added a comment.

I like the idea of this, giving control to the user to figure out the best way to manipulate loops (even if they end up shooting themselves in the foot with it :) )

What does this mean for the clang side of this patch? Has the user-visible loop-naming scheme changed, or will it be mapped to these matadatas?

What follows is mostly a number of small Nits:



================
Comment at: docs/TransformMetadata.rst:23
+transformations it should apply. This can be additional knowledge that
+cannot be derived from he emitted IR, or directives passed from the
+user/programmer. OpenMP pragmas are an example for the latter.
----------------
Nit: from the emitted IR


================
Comment at: docs/TransformMetadata.rst:53
+Loop Distribution, etc.). They can either be a hint to the optimizer
+that a tranformation might be beneficial, instruction to use a specific
+option, or convey a mandatory declaration by the user ('forced'; e.g.
----------------
Nit: transformation 


================
Comment at: docs/TransformMetadata.rst:82
+After a transformation is applied, follow-up attributes are set on the
+transfomed and/or new loop(s). This allows additional attributes
+including followup-transformations to be specified. Specifying multiple
----------------
Nit: transformed


================
Comment at: docs/TransformMetadata.rst:87
+``llvm.loop.vectorize.enable`` and ``llvm.loop.unroll.enable`` are
+specified at the same time, unrolling may occure either before or after
+vectorization.
----------------
Nit: occur


================
Comment at: docs/TransformMetadata.rst:252
+``llvm.loop.unroll_and_jam.followup_remainder_inner`` sets the loop
+attributes of the inner remainder loop. If not specified it will habe
+the attributes of the original inner loop. It the outer remainder loop
----------------
Nit: have


================
Comment at: docs/TransformMetadata.rst:343
+
+Currently, the ``LoopInterchange`` pass does not use any metadata.
+
----------------
Should we fix this? Will it work as expected with nonforced, if it was enabled?


================
Comment at: include/llvm/LinkAllPasses.h:221
       (void) llvm::createScalarizeMaskedMemIntrinPass();
+      (void)llvm::createWarnMissedTransformationsPass();
 
----------------
Nit: Space I guess? I think this file could do with a clang-formatting


================
Comment at: include/llvm/Transforms/Scalar/WarnMissedTransforms.h:35
+Pass *createWarnMissedTransformationsPass();
+void initializeWarnMissedTransformationsLegacyPass(PassRegistry &);
+} // end namespace llvm
----------------
Are these two needed here, if they are declared in other places?


================
Comment at: lib/Transforms/Scalar/LoopUnrollAndJamPass.cpp:336
+  MDNode *OrigOuterLoopID = L->getLoopID();
+  Loop *InnerLoop = L->getSubLoops().front();
+  MDNode *OrigInnerLoopID = InnerLoop->getLoopID();
----------------
This is called SubLoop here


Repository:
  rL LLVM

https://reviews.llvm.org/D49281





More information about the llvm-commits mailing list