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

[Michael Kruse via Phabricator via llvm-commits]

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


================
Comment at: docs/TransformMetadata.rst:87
+``llvm.loop.vectorize.enable`` and ``llvm.loop.unroll.enable`` are
+specified at the same time, unrolling may occur either before or after
+vectorization.
----------------
hfinkel wrote:
> Why is this a useful feature? Should we allow only one transformation per node?
It's not a new feature, but effectively the current behavior. ("is possible for compatibility
reasons"). I'd indeed prefer to crash if multiple transformations are applied to avoid undefined behavior, which unfortunately is a breaking change.


================
Comment at: docs/TransformMetadata.rst:103
+all attributes from the original loop excluding its loop vectorizer
+attributes. To avoid this, an empty followup attribute can be used, e.g.
+
----------------
hfinkel wrote:
> This leaves open the question of whether the vectorizer adds the 'isvectorized' attribute when a follow-up is specific. It should, right?
I think it should not, but the metadata gives complete control over which attributes will be in the new loop to the metadata. If a frontend wants to apply a transformation twice, it should be able to do so.

I think the paragraph says that no such additional implicit attributes are added when a followup is specified: 
> If no followup is specified, ...

I added "If, and only if," to make it even clearer.


================
Comment at: docs/TransformMetadata.rst:152
+``llvm.loop.vectorize.followup_vectorized`` will set the attributes for
+the vectorized loop. If not specified, ``llvm.loop.isvectorized`` is
+combined with the original loop's attributes to avoid it being
----------------
hfinkel wrote:
> Why would isvectorized not always be provided?
I don't know how LoopVectorize behaves when encountering vector instructions, but I want to avoid the vagueness of some passes adding implicit metadata in some situation. The IR should have the control over whether a transformation is applied multiple times.


================
Comment at: include/llvm/Transforms/Utils/LoopUtils.h:534
+
+  /// The transformation is necessary for correctness. Unlike general loop
+  /// metadata, it must not be dropped. If the transformation could not be
----------------
hfinkel wrote:
> We can't have metadata necessary for correctness. 'ForcedByUser' is fine to indicate that the user should receive a warning if the transformation cannot be performed.
[comment] This indicates that using metadata for user-directed loop-transformation #pragmas is a leaky abstraction.


================
Comment at: lib/Transforms/Scalar/LoopUnrollAndJamPass.cpp:365
+  // To assign the loop id of the epilogue, assign it before unrolling it so it
+  // is applied to every inner loop of the epilogue. We later apply the loop ID
+  // for the jammed inner loop.
----------------
hfinkel wrote:
> So each inner loop gets the same id? That doesn't sound right.
`LoopID` is a misnomer. A LoopID is neither unique (multiple loops having the same LoopID, e.g. because LoopVersioning of any pass not aware of loops copied the BBs of a loop; there is even a regression test for the behaviour with non-unique LoopID) nor identifying (adding/removing attributes in the LoopID MDNode will create a new MDNode; see D52116 for a fix for llvm.loop.parallel_accesses assuming this property).

Fixing LoopID to be identifier-like is not possible with the current MDNode structure and would require to make any pass that copies code to be aware of LoopIDs. It would be easier to not assume that LoopID has any identifying properties. I am open to rename 'LoopID' to something else.


Repository:
  rL LLVM

https://reviews.llvm.org/D49281