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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel accepted this revision.
hfinkel added a comment.
This revision is now accepted and ready to land.

A few additional comments. Otherwise, this LGTM. When @dmgreen is happy with the unrolling changes, I think you're good to go.



================
Comment at: docs/LangRef.rst:5124
+other transformations impossible. Mandatory loop canonicalizations such
+as loop rotation are still applied.
+See :ref:`transformation-metadata` for details.
----------------
Maybe add, "It is recommended to use this metadata when using any of the other llvm.loop.* metadata to direct specific transformations."


================
Comment at: docs/TransformMetadata.rst:400
+transformation pass should be reported to the user. The transformation
+passes themselves cannot be responsible for this reporting because there
+might not be in the pipeline, there might be multiple passes able to
----------------
there -> they


================
Comment at: lib/Transforms/Scalar/LoopDistribute.cpp:87
+    "llvm.loop.distribute.followup_coincident";
+const char *const LLVMLoopDistributeFollowuSequential =
+    "llvm.loop.distribute.followup_sequential";
----------------
This should say Followup, not Followu, I suppose.


================
Comment at: lib/Transforms/Scalar/WarnMissedTransforms.cpp:32
+                                          L->getStartLoc(), L->getHeader())
+        << "loop not unrolled: failed explicitly specified loop unrolling");
+  }
----------------
Here and below, explicitly specified should have a hyphen (it is a compound adjective):

  explicitly-specified loop unrolling

that having been said, I'd prefer a different phrasing all together. These are end-user visible messages, and I think that we can make these slightly more user friendly. How about this:

  
  "loop not unrolled: the optimizer was unable to perform the requested transformation"

(and similar for the others)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D49281