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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

I am thinking about adding a `LoopMetadataTacker` (sort of a combination of LoopVectorizeHints and AssumptionTracker) analysis pass which would centralize the interpretation of that metadata and avoid the linear search through the metadata list when looking up a specific attribute.



================
Comment at: lib/Transforms/Scalar/LoopUnrollAndJamPass.cpp:194
 
+  TransformationMode EnableMode = hasUnrollAndJamTransformation(L);
+  if (EnableMode & TM_Disable) {
----------------
dmgreen wrote:
> This code will need rebasing. There is a check earlier that looks for disable metadata that could be replaced by this. Look for HasUnrollDisablePragma/HasUnrollAndJamDisablePragma. If the same was done for unrolling, I think that would remove the need for the IgnoreUser (although your comment about it is probably still true).
I'd push towards refectoring-out the common parts of `computeUnrollCount` used by LoopUnroll and LoopUnrollAndJam. Currently `computeUnrollCount` uses lots of settings meant for LoopUnroll (`llvm.loop.unroll.` metadata which should not exist anymore, OptimizationRemarkMissed specific to LoopUnroll, `-unroll-count`, `PartialThreshold`, handling of full unroll, loop peeling that UnrollAndJam does not support, being used in a single call by UnrollAndJam for two different things: determining `ExplicitUnroll` (i.e. is normal unroll is forced) and the unroll-and-jam count). It's hard to understand the subtleties between those codes.

I gave up at some point and added the `IgnoreUser` flag to make test cases pass.


================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:543
+                                      AssumptionCache *AC, bool PreserveLCSSA,
+                                      Loop **ResultLoop) {
   LLVM_DEBUG(dbgs() << "Trying runtime unrolling on Loop: \n");
----------------
hiraditya wrote:
> What is the rationale of using pointer to a pointer here? If we want to assign to ResultLoop, then maybe we can just return ResultLoop and bool as a pair.
If the Result loop is not needed, one can pass `nullptr` (which is the default argument). Returning `std::pair` will require more changes.


================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:912
 
+  auto UnrollResult = LoopUnrollResult ::Unmodified;
   if (remainderLoop && UnrollRemainder) {
----------------
hiraditya wrote:
> nit: space
This is done by `clang-format`. It try not to fight its decisions and hope for future improvement.


================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:1384
+  bool InheritAllAttrs = !InheritOptionsExceptPrefix;
+  bool InheritSomeAttrs =
+      InheritOptionsExceptPrefix && InheritOptionsExceptPrefix[0] != '\0';
----------------
dmgreen wrote:
> Maybe InheritSomeAttrs -> InheritNonExceptAttrs?
Avoiding double negation here.


Repository:
  rL LLVM

https://reviews.llvm.org/D49281