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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Some extra tests for nonforced + a pragma would be good to see.

I'm not much of an expert on the vectoriser changes here.



================
Comment at: docs/LangRef.rst:5245
+
+'``llvm.loop.unroll_and_jam.followup_outer``' Metadata
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
----------------
These can now move down with the other unroll_and_jam metadata


================
Comment at: docs/LangRef.rst:5263
+This metadata defines which attributes the epilogue of the outer loop
+will have. This loops is usually unrolled, meaning there is no such
+loop. This attribute will be ignored in this case. See
----------------
Nit: This loop


================
Comment at: docs/TransformMetadata.rst:343
+
+Currently, the ``LoopInterchange`` pass does not use any metadata.
+
----------------
Meinersbur wrote:
> dmgreen wrote:
> > Should we fix this? Will it work as expected with nonforced, if it was enabled?
> Yes, but in a separate patch that adds interchange-specific metadata. LoopInterchange currently is not enabled by default and does not modify metadata so its interaction with other transformation is less significant.
Yep. Certainly a separate patch.


================
Comment at: docs/TransformMetadata.rst:71
+For instance, the following example avoids that the loop is altered
+before being vectorized, for instance being unrolled.
+
----------------
Nit: Maybe change the second "for instance"


================
Comment at: docs/TransformMetadata.rst:163
+full unrolling, partial unrolling of a loop with constant trip count or
+runtime unrolling of a loop with a trip count unknown at compile-time.
+
----------------
Do you think it's worth mentioning unroll.count and unroll.disable etc, before jumping into the followup metadata?


================
Comment at: docs/TransformMetadata.rst:206
+unroll factor if 2). Currently, it does not support a fallback version
+when the transformation is unsafe.
+
----------------
Again, could mention unroll_and_jam.count and enable/disable.


================
Comment at: docs/TransformMetadata.rst:264
+from the non-vectorizable part (which otherwise would make the entire
+loop non-vectorizable). Conceptually, it transforms a loop such as
+
----------------
Again, maybe describe other metadata first?


================
Comment at: docs/TransformMetadata.rst:395
+that is executed later and thus leftover. For instance, a loop nest
+cannot be distributed and then interchanged with the current pass pass
+pipeline. The loop distribution will execute, but there is no loop
----------------
Nit: pass pass


================
Comment at: lib/Transforms/Scalar/LoopUnrollAndJamPass.cpp:194
   bool PragmaEnableUnroll = HasUnrollAndJamEnablePragma(L);
+  if (!PragmaEnableUnroll && hasDisableAllTransformsHint(L))
+    return false;
----------------
Same as unroll. What if PragmaCount and  hasDisableAllTransformsHint?


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:753
+  if (!PragmaEnableUnroll && hasDisableAllTransformsHint(L))
+    return false;
   bool ExplicitUnroll = PragmaCount > 0 || PragmaFullUnroll ||
----------------
What if it has PragmaCount and hasDisableAllTransformsHint? Should that not enable too?


================
Comment at: test/Transforms/LoopUnroll/unroll-pragmas_transform.ll:1
+; RUN: opt < %s -loop-unroll -pragma-unroll-threshold=1024 -S | FileCheck -check-prefixes=CHECK,REM %s
+; RUN: opt < %s -loop-unroll -loop-unroll -pragma-unroll-threshold=1024 -S | FileCheck -check-prefixes=CHECK,REM %s
----------------
This file look sensible on it's own and I think looks OK to be committed separately. (Apart from the nit below)


================
Comment at: test/Transforms/LoopUnroll/unroll-pragmas_transform.ll:5
+;
+; Run loop unrolling twice to verify that loop unrolling metadata is properly
+; removed and further unrolling is disabled after the pass is run once.
----------------
Nit: Is this sentence still true?


Repository:
  rL LLVM

https://reviews.llvm.org/D49281