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

Max Kazantsev via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Mar 27 00:24:35 PDT 2018


mkazantsev 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:
> 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.


https://reviews.llvm.org/D44880





More information about the llvm-commits mailing list