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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:323
+struct PragmaInfo {
+  bool UserUnrollCount;
+  bool PragmaFullUnroll;
----------------
Nit: bool and unsigned don't get initialized, so to avoid nasty nondeterministic bugs, easiest to assign them on declaration, here, i.e. bool UserUnrollCount = false; unsigned PragmaCount = 0; etc. This improves also maintainability, because it's *much* easier for folks working with the code to follow the same pattern if they add more fields, and also figure out where the  object is initialized (less reading required, basically)


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:763
+    if (CountOverwrite)
+      return (uint64_t)(LoopSize - UP.BEInsns) * CountOverwrite + UP.BEInsns;
+    else
----------------
do static_cast<uint64_t>(same stuff)

same below


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:889
 // refactored into it own function.
+
 bool llvm::computeUnrollCount(
----------------
delete this line, it's spurious


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:899
   UnrollCostEstimator UCE(*L, LoopSize);
+  PragmaInfo PInfo;
+  Optional<unsigned> UnrollFactor;
----------------
One thing to consider: if PragmaInfo are just carriers of those fields, and (currently) they are meant to be set once, I'd make the code even more maintainable if:
- the fields were const
- the struct had a ctor where you pass them and set them

that way, it's very clear that a PragmaInfo object has intentionally initialized fields, and also that they can't ever be changed.


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:936
+
+    if (UserUnrollCount | (PragmaCount > 0)) {
+      UP.AllowExpensiveTripCount = true;
----------------
why binary or? i.e. did you mean || ?


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:968
   // In addition we only unroll small upper bounds.
+
   unsigned FullUnrollMaxTripCount = MaxTripCount;
----------------
remove spurious line


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:1004
     UP.Count = 1;
+
     return ExplicitUnroll;
----------------
remove spurious line


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:1044
     }
-    if (UP.Count > UP.MaxCount)
-      UP.Count = UP.MaxCount;
-    if ((PragmaFullUnroll || PragmaEnableUnroll) && TripCount &&
-        UP.Count != TripCount)
-      ORE->emit([&]() {
-        return OptimizationRemarkMissed(DEBUG_TYPE,
-                                        "FullUnrollAsDirectedTooLarge",
-                                        L->getStartLoc(), L->getHeader())
-               << "Unable to fully unroll loop as directed by unroll pragma "
-                  "because "
-                  "unrolled size is too large.";
-      });
-    LLVM_DEBUG(dbgs() << "  partially unrolling with count: " << UP.Count
-                      << "\n");
+
     return ExplicitUnroll;
----------------
line


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:1047
   }
+
   assert(TripCount == 0 &&
----------------
line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106001