[PATCH] D44919: [LoopUnroll][NFC] Remove redundant canPeel check

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

Thanks for clarification @iajbar ! I see what happens. We enter `computeUnrollCount` but do not reach `computePeelCount` because we exit earlier (for example, deciding to make full unrolling). The further logic assumes that if `UP.PeelCount` is set, then we will do peeling and nothing else. In sense of that, your patch https://reviews.llvm.org/D44880 contains a bug that I haven't noticed on review.

Specifically, even without this patch, your patch will lead to following:

1. `UP.Count` will be forcefully set to some value;
2. Unroller may decide to make full unrolling;

2a) If the loop can be peeled, then it will first peel off 2 iterations and THEN make full unrolling of what remains, which makes no sense (yet it seems functionally correct, but it will still be waste of compile time).
2b) If the loop cannot be peeled, then it will be rejected by peelLoop and then passed for unrolling, or, with this patch applied, lead to assertion failure.

I don't think that peeling 2 iterations first and making full unrolling after is what you expect to do (yet I don't see any correctness problems about it).

I suggest to add `canPeel` check to your patch and re-land it to avoid this situation. In this case you will at least not crash on unpeelable loops and will not set peel count for it. I think it has some sense by itself, because setting peel count to unpeelable loops may be misleading.

But it will not solve the potential compile time issue with making peeling first and unrolling after, which exist in your patch independently of my patch.

If this unroll-after-peeling situation seems unimportant for you, please add `canPeel` check and re-land the patch (consider this version approved by me).
If it is important, then we should find another way to inform the peel pass that you want to peel off 2 iterations ONLY WHEN it actually decides to do peeling and not full/user-forced/pragma-forced unrolling.

I hope my evaluation was useful to you.

Good luck,
Max


Repository:
  rL LLVM

https://reviews.llvm.org/D44919