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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:803
+    // default value 0.
+    computePeelCount(L, LoopSize, UP, TripCount, SE);
   if (UP.PeelCount) {
----------------
mkazantsev wrote:
> mkazantsev wrote:
> > 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.
> I have submitted https://reviews.llvm.org/D44919 for review to get rid of this second check, I believe this is how it should work.
How about changing computePeelCount to take the maximum of the passed in peelCount and the compute peel count? That seems to be in line with how other unroll settings are handled.

For example, if computePeelCount finds a higher peel count, shouldn't that one be used?

Also note that even settings like UnrollForcePeelCount are only applied after the other calculations could not find a peel count.


https://reviews.llvm.org/D44880