[PATCH] D64972: [Loop Peeling] Do not close further unroll/peel if profile based peeling was not used

Serguei Katkov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jul 30 19:36:29 PDT 2019


skatkov added a comment.

In D64972#1606836 <https://reviews.llvm.org/D64972#1606836>, @reames wrote:

> Having both the enable flag and the AlreadyPeeled variable is really confusing.  Is there a way we could combine them?  Maybe replace the boolean with the AlreadyPeeledViaProfiling count or something?


To me the flag AllowPeeling is to disable peeling at all while AlreadyPeeledViaProfiling is for disablement of part of the peeling.
In this term I would update the comment for AllowPeeling:
/// Allow peeling off loop iterations for loops with low dynamic tripcount.
to 
/// Allow peeling off loop iterations.
taking into account that it is not now true (before my change as well).

If we really want to combine these options it makes sense to introduce enum with possible levels of peeling (say None, NonProfileBased, All).
Coding this enum with integer is not something really reducing the confusion.
However this enum will not be aligned with different unroll options. Also this will require some modifications in options handling - Different ways to set the desired level of peeling needs to support this new enum.

Let me know if we really want to introduce this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64972/new/

https://reviews.llvm.org/D64972





More information about the llvm-commits mailing list