[PATCH] D25963: [LoopUnroll] Implement profile-based loop peeling

[Michael Kuperstein via llvm-commits]

mkuper added a comment.

Thanks, David.



================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:724
     unsigned MaxTripCount, unsigned &TripMultiple, unsigned LoopSize,
-    TargetTransformInfo::UnrollingPreferences &UP, bool &UseUpperBound) {
+    unsigned &PeelCount, TargetTransformInfo::UnrollingPreferences &UP,
+    bool &UseUpperBound) {
----------------
davidxl wrote:
> Should PeelCount be a member of UnrollingPreferences class?
I think not, but I don't really know.
To the best of my understanding, the main point of UnrollingPreferences is to give targets the ability to override the defaults, and I'm not sure this is useful here.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:887
+  // If the user provided a peel count, use that.  
+  bool UserPeelCount = UnrollForcePeelCount.getNumOccurrences() > 0;
+  if (UserPeelCount) {
----------------
davidxl wrote:
> I think all the logic here should probably be wrapped in a helper function "computeOptimalPeelCount".
Maybe. That was the idea behind getPeelCount() - but I thought to leave the forcing logic here, where it is for all the other user knobs.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:904
+  if (UP.AllowPeeling && L->getHeader()->getParent()->getEntryCount()) {
+    PeelCount = getPeelCount(L);
+
----------------
davidxl wrote:
> getPeelCount looks like a simple wrapper that is only used once. Probably get rid of it.
Right, it originally contained the code that now lives in getLoopEstimatedTripCount(), but I decided to factor that out because that's more generally useful (e.g. I can see it used in the vectorizer).


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:63
+
+static void cloneLoopBlocksForPeeling(
+    Loop *L, bool FirstCopy, BasicBlock *InsertTop, BasicBlock *InsertBot,
----------------
davidxl wrote:
> Is there common utility to do body cloning?
Not really.

This is based on CloneLoopBlocks() in LoopUnrollRuntime, but it's sufficiently different so that sharing code didn't really make sense to me. The main loop unrolling code also has its own, more complicated, version of this.

The basic loop copying code is fairly straightforward
```
for (LoopBlocksDFS::RPOIterator BB = BlockBegin; BB != BlockEnd; ++BB) {
  BasicBlock *NewBB = CloneBasicBlock(*BB, VMap, ".", F);
  NewBlocks.push_back(NewBB);
  VMap[*BB] = NewBB;
}
remapInstructionsInBlocks(NewBlocks, VMap);
```

But the different versions do different fixups on the fly - hookinh up inputs, outputs and branches, VMap changes, updating ScalarEvolution and/or LoopInfo, etc.


https://reviews.llvm.org/D25963