[PATCH] D36258: Disable loop peeling during full unrolling pass.

Thu Aug 3 06:06:34 PDT 2017

tejohnson marked an inline comment as done.
tejohnson added a comment.

In https://reviews.llvm.org/D36258#830100, @chandlerc wrote:

> In https://reviews.llvm.org/D36258#830096, @davidxl wrote:
>
> > Perhaps it is cleaner to
> >
> > 1. pass the information whether it is a full unroll to tryToUnroll
> > 2. add a internal option FullUnrollAllowPeeling and make it off by default?  This will be similar to UnrollAllowPeeling flag, but takes precedence if it is full unroll.
>
>
> While I'm somewhat fond of refactoring these interfaces to be less of a mess, would it be better in a follow-up patch? I would also hope it can tackle more than just peeling but encompass several of the numerous parameters.

Right, I wanted to be consistent with what is there for other unrolling features and how we disable them during full unrolling.

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:204
     UP.UpperBound = *UserUpperBound;
+  if (UserAllowPeeling.hasValue())
+    UP.AllowPeeling = *UserAllowPeeling;
----------------
davidxl wrote:
> chandlerc wrote:
> > davidxl wrote:
> > > Move this closer to line 188 above?
> > I think this location is more consistent with the surrounding code... I'm reluctant to deviate here from what seems like a very consistent pattern.
> Right. They all should be interleaved so that settings to the same flag should be side by side, but that is not relevant here.
Right, I put it here to be consistent.

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:1289
+        /*Threshold*/ None, AllowPartialParam, RuntimeParam, UpperBoundParam,
+        /*AllowPeeling*/ None);
     Changed |= CurChanged;
----------------
chandlerc wrote:
> Shouldn't this pass `AllowPeeling` since you added that variable above?
Woops, added that variable then realized I could just pass None directly and forgot to remove it. I'll remove the variable.

https://reviews.llvm.org/D36258