[PATCH] D105173: Initial refactoring

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added a comment.

Could you update the commit message (both git and here) to say what the patch accomplishes, i.e. "[NFC] factor out unrolling decision logic" or something like that - nfc because it should just be a refactoring. The message can clarify why - e.g. so it may simplify exploring alternative decision-making mechanisms.



================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:756
 
+bool unrollingLogic(bool FullOrPartial, Loop *L, const TargetTransformInfo &TTI,
+                    DominatorTree &DT, ScalarEvolution &SE,
----------------
use verb names for functions, e.g. shouldUnroll. That also explains what the return means.

Also add a comment above it, like computeUnrollCount has.

does MaxTripCount need to be passed by-ref?

could the return of this function be rather a more complex object, where the TripCount and TripMultiple were returned - I haven't looked yet at how it's used, is it that the caller proposes such values, and this adjusts them?


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:767
+
+  bool UserUnrollCount = UnrollCount.getNumOccurrences() > 0;
+
----------------
nit: if UserUnrollCount doesn't change later, just const - it. The reader then understands this is meant to help with understanding the code. Same below.


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:828
-
-  // 3rd priority is full unroll count.
-  // Full unroll makes sense only when TripCount or its upper bound could be
----------------
why remove the comment?


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:960
 
-  // Check for explicit Count.
-  // 1st priority is unroll count set by "unroll-count" option.
-  bool UserUnrollCount = UnrollCount.getNumOccurrences() > 0;
-  if (UserUnrollCount) {
-    UP.Count = UnrollCount;
-    UP.AllowExpensiveTripCount = true;
-    UP.Force = true;
-    if (UP.AllowRemainder && UCE.getUnrolledLoopSize(UP) < UP.Threshold)
-      return true;
-  }
+  bool tryFullUnroll = true;
 
----------------
capital 'T'.

And/but, if you want to do this for clarity wrt what the params mean, 3 options:

1) no variable name. Instead (the way commonly seen in the codebase):

unrollingLogic(/*TryFullUnroll=*/true, ...)

2) just do this (but it's not common in this codebase)

const bool DoFullUnroll = true;
const bool DoPartialUnroll = false;

3) define an enum class:

enum class UnrollingOption : bool {
  Full,
  Partial
};

...you see where this is going



================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:968
 
   // 4th priority is loop peeling.
   computePeelCount(L, LoopSize, PP, TripCount, SE, UP.Threshold);
----------------
isn't clear where the 1-3 priorities are, just add a comment about that


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:977
+
+  ExplicitUnroll = unrollingLogic(tryFullUnroll, L, TTI, DT, SE, EphValues, ORE,
+                                  MaxOrZero, LoopSize, TripMultiple, TripCount,
----------------
it seems like the only difference between the first call and this is whether to try full unroll. We could, instead, return an enum from <what's currently called unrollingLogic>. 

*or* factor that function in 2: shouldFullyUnroll, shouldPartiallyUnrol


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:1008
   // Reduce count based on the type of unrolling and the threshold values.
-  UP.Runtime |= PragmaEnableUnroll || PragmaCount > 0 || UserUnrollCount;
+  // UP.Runtime |= PragmaEnableUnroll || PragmaCount > 0 || UserUnrollCount;
+
----------------
don't comment code, remove it instead (if you don't need it)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105173