[PATCH] D44880: [Hexagon] peel loops with runtime small trip counts

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:803
+    // default value 0.
+    computePeelCount(L, LoopSize, UP, TripCount, SE);
   if (UP.PeelCount) {
----------------
iajbar wrote:
> mkazantsev wrote:
> > I don't think it is correct. `computePeelCount` will set PeelCount to 0 if the loop is unpeelable, and you are forcing it to some value without checking this fact.
> There is another call to canPeel() in peelLoop().  In the case of a target sets the peel count to Non zero, then the loop won’t get peel if it’s invalid to be peeled.
1) This still need a test that ensures this behavior preserves.
2) Actually, to me it looks quite strange, and I'd rather prefer having `canPeel` in `peelLoop` as assert in future. It makes no sense to make the same set of checks in two different places that go after one another.

I suggest you to move your logic inside `computePeelCount` and return the number of peeled iterations set by your platform-specific code *after* you check that the loop can be peeled. This will allow to replace the check in `peelLoop` with assertion later.


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:480
+  // Only try to peel innermost loops.
+  if (!L->empty())
+    return false;
----------------
1) Why it is not a part of canPeel?
2) This is not connected to your logic about setting peel count in target. This should be a separate patch with justification and tests for this very change.


https://reviews.llvm.org/D44880