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

Fri Sep 21 08:44:33 PDT 2018

hfinkel added inline comments.

================
Comment at: docs/LangRef.rst:5092
+
+This metadata disables any non-explicit transformation of this loop,
+meaning no heuristic is applied that tries to optimize this loop. See
----------------
This is too strong (see comment below).

================
Comment at: docs/LangRef.rst:5165
+epilogue is not vectorized and is executed when either the vectorized
+loop is not known to preserving semantics (because e.g. two arrays it
+processes are found to alias by a runtime check) or for the last
----------------
preserving -> preserve

================
Comment at: docs/LangRef.rst:5165
+epilogue is not vectorized and is executed when either the vectorized
+loop is not known to preserving semantics (because e.g. two arrays it
+processes are found to alias by a runtime check) or for the last
----------------
hfinkel wrote:
> preserving -> preserve
e.g., because two ...

================
Comment at: docs/LangRef.rst:5167
+processes are found to alias by a runtime check) or for the last
+iterations that do not fill a complete vector lane. See
+:ref:`Transformation Metadata <transformation-metadata>` for details.
----------------
vector lane -> set of vector lanes

================
Comment at: docs/LangRef.rst:5173
+
+Attributes in the metadata will be added to the vectorized as well as to
+the remainder loop. See
----------------
added to both the vectorized and remainder loop

================
Comment at: docs/TransformMetadata.rst:14
+LLVM transformation passes can be controlled by attaching metadata to
+the code to transform. By default passes will apply a heuristic on
+whether to apply a transformation and using which parameters.
----------------
By default, transformation passes use heuristics to determine whether or not to perform transformations, and when doing so, other details of how the transformations are applied (e.g., which vectorization factor to select). 

================
Comment at: docs/TransformMetadata.rst:17
+Transformations are usually applied conservatively, i.e. will only be
+applied if it is unlikely to cause any slowdown for any workload, or
+such a slowdown would be minor. Therefore most optimizations will be
----------------
As stated, this is untrue (for -O3). For -O3, we only require a likely speedup across many workloads (and slowdowns be unlikely). This is why, for example, under -O3, we can vectorize with runtime checks. How about this wording:

Unless the optimizer is otherwise directed, transformations are applied conservatively. This conservatism generally allows the optimizer to avoid unprofitable transformations, but in practice, this results in the optimizer not applying transformations that would be highly profitable.

================
Comment at: docs/TransformMetadata.rst:22
+Frontends can give additional hints to LLVM passes on which
+transformations it should apply. This can be additional knowledge that
+cannot be derived from the emitted IR, or directives passed from the
----------------
it -> they

================
Comment at: docs/TransformMetadata.rst:24
+cannot be derived from the emitted IR, or directives passed from the
+user/programmer. OpenMP pragmas are an example for the latter.
+
----------------
for -> of

================
Comment at: docs/TransformMetadata.rst:51
+
+Some attributes describe code transformations (Unrolling, Vectorizing,
+Loop Distribution, etc.). They can either be a hint to the optimizer
----------------
Unrolling, etc. - no need to capitalize.

================
Comment at: docs/TransformMetadata.rst:54
+that a transformation might be beneficial, instruction to use a specific
+option, or convey a mandatory declaration by the user ('forced'; e.g.
+``#pragma clang loop`` or ``#pragma omp simd``).
----------------
We should be careful with the language here. As any of these can be dropped without changing the semantics of the code, nothing here is "mandatory". How about saying, ", or convey a specific request from the user"

================
Comment at: docs/TransformMetadata.rst:58
+If a transformation is forced but cannot be carried-out for any reason,
+an optimization missed warning must be emitted. Semantic information
+such as a transformation being safe (e.g.
----------------
optimization-missed warning

================
Comment at: docs/TransformMetadata.rst:60
+such as a transformation being safe (e.g.
+``llvm.mem.parallel_loop_access``) is separate.
+
----------------
I know what you mean by "separate", but I think it's better to say:

is separate. -> can be unused by the optimizer without generating a warning.

================
Comment at: docs/TransformMetadata.rst:68
+explicitly disable an unknown number of passes, the attribute
+``llvm.loop.disable_nonforced`` disables all non-forced transformation.
+
----------------
This wording is too strong. I think that we need to draw a distinction here between (at least) three classes of transformations:

 1. canonicalizing transformations
 2. (cost-model-driven) restructuring transformations
 3. low-level (target-specific) transformations (e.g., using ctr-register-based loops on PowerPC)

I believe that this pragma should only affect those in class (2). Canonizalizing transformations are always performed (when optimizing at all), and low-level transformations are beyond the reach of this kind of metadata.

I'd recommend using the wording that this metadata disables, "optional, high-level, restructuring transformations."

================
Comment at: docs/TransformMetadata.rst:70
+
+The following example avoids that the loop is altered
+before being vectorized, for instance being unrolled.
----------------
avoids that the loop is altered -> avoids the loop being altered

================
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.
----------------
Why is this a useful feature? Should we allow only one transformation per node?

================
Comment at: docs/TransformMetadata.rst:90
+
+As an example, the following instructs a loop being vectorized and only
+then unrolled.
----------------
loop being vectorized -> loop to be vectorized

================
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.
+
----------------
This leaves open the question of whether the vectorizer adds the 'isvectorized' attribute when a follow-up is specific. It should, right?

================
Comment at: docs/TransformMetadata.rst:120
+
+Transformation options are specific for each transformation. In the
+following we present the model for each LLVM loop optimization pass and
----------------
for -> to

================
Comment at: docs/TransformMetadata.rst:121
+Transformation options are specific for each transformation. In the
+following we present the model for each LLVM loop optimization pass and
+the metadata to influence them.
----------------
comma after following

================
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
----------------
Why would isvectorized not always be provided?

================
Comment at: docs/TransformMetadata.rst:300
+    }
+
+``llvm.loop.distribute.followup_coincident`` sets the loop attributes of
----------------
where 'rtc' is the generated runtime safety check.

================
Comment at: docs/TransformMetadata.rst:390
+Forced transformations that have not been applied after the last
+transformation pass must be reported to the user. The transformation
+passes themselves cannot be responsible because they might not be in the
----------------
must be -> should be

================
Comment at: docs/TransformMetadata.rst:391
+transformation pass must be reported to the user. The transformation
+passes themselves cannot be responsible because they might not be in the
+pipeline, they might be multiple passes being able to apply a
----------------
responsible for this reporting

================
Comment at: docs/TransformMetadata.rst:392
+passes themselves cannot be responsible because they might not be in the
+pipeline, they might be multiple passes being able to apply a
+transformation (e.g. ``LoopInterchange`` and Polly) or a transformation
----------------
they might -> there might

================
Comment at: docs/TransformMetadata.rst:392
+passes themselves cannot be responsible because they might not be in the
+pipeline, they might be multiple passes being able to apply a
+transformation (e.g. ``LoopInterchange`` and Polly) or a transformation
----------------
hfinkel wrote:
> they might -> there might
being able - > able

================
Comment at: docs/TransformMetadata.rst:394
+transformation (e.g. ``LoopInterchange`` and Polly) or a transformation
+attribute is 'hidden' inside another passes' followup attribute.
+
----------------
is -> may be

(keep the entire list in the hypothetical)

================
Comment at: docs/TransformMetadata.rst:410
+Future versions of LLVM may fix this by executing transformations in a
+fixpoint loop.
----------------
in a fixedpoint loop -> using a dynamic ordering

(not to be too prescriptive)

================
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
----------------
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 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.
----------------
So each inner loop gets the same id? That doesn't sound right.

Repository:
  rL LLVM

https://reviews.llvm.org/D49281