[PATCH] D106001: [WIP][NFC] factor out unrolling decision logic

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:761
 
-// Returns true if unroll count was set explicitly.
-// Calculates unroll count and writes it to UP.Count.
-// Unless IgnoreUser is true, will also use metadata and command-line options
-// that are specific to to the LoopUnroll pass (which, for instance, are
-// irrelevant for the LoopUnrollAndJam pass).
-// FIXME: This function is used by LoopUnroll and LoopUnrollAndJam, but consumes
-// many LoopUnroll-specific options. The shared functionality should be
-// refactored into it own function.
-bool llvm::computeUnrollCount(
-    Loop *L, const TargetTransformInfo &TTI, DominatorTree &DT, LoopInfo *LI,
+std::pair<bool, bool> shouldFullPragmaUnroll(
+    Loop *L, const TargetTransformInfo &TTI, DominatorTree &DT,
----------------
Use a struct with a name, not a pair of bools. It is much harder to determine what the bools mean this way, if you return an object of type `struct SomeGoodName { bool InfoA; bool InfoB; }` it is clear what the bools mean.
Same in other places.


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:901
+  const bool ExplicitUnroll = PragmaCount > 0 || PragmaFullUnroll ||
+                              PragmaEnableUnroll || UserUnrollCount;
   if (TripCount) {
----------------
We have duplication here. Can you query this once and put everything into a "info struct" and then pass that to all the functions here.

Make the functions static if you can.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106001