[PATCH] D19553: Unroll pass restructure.

[Evgeny Stupachenko via llvm-commits]

evstupac added a comment.

Let's wait for other suggestions/comments. However the patch is here 20 days without significant changes.
So there could be more comments only after commit.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:537-544
@@ -549,1 +536,10 @@
+  bool UserUnrollCount = UnrollCount.getNumOccurrences() > 0;
+  if (UserUnrollCount) {
+    UP.Count = UnrollCount;
+    UP.AllowExpensiveTripCount = true;
+    UP.Force = true;
+    if (UP.AllowRemainder &&
+        (LoopSize - 2) * UP.Count + 2 < UP.Threshold)
+      return true;
+  }
 
----------------
mzolotukhin wrote:
> evstupac wrote:
> > mzolotukhin wrote:
> > > I find it a bit confusing that we change values in `UP` even if we don't exit the function. Can we rewrite it to something like:
> > > ```
> > > If (condition1) {
> > >   UP = ...;
> > >   return true; // We only change UP when we are going to return
> > > }
> > > if (condition2) {
> > >   UP = ...;
> > >   return true;
> > > }
> > > ```
> > > as opposed to
> > > ```
> > > if (condition1) {
> > >   UP = ...
> > >   if (condition)
> > >     return true;
> > >   // Here we changed UP, but didn't return
> > > }
> > > ```
> > We can do so, but still we need to enable all the properties somehow.
> > For "condition1" it will look like:
> > 
> >   If (condition1) {
> >     UP = ...;
> >     return true; // We only change UP when we are going to return
> >   }
> >   if (condition1)
> >     UP = ...;
> > 
> > That is because if user specified count that exceed threshold we are reducing it, but still allow some certain level of unrolling (Runtime, Full, Force....) 
> > 
> > Personally I'd better disable unroll and warn user in that case (telling how to enable specified unroll by increasing pragma threshold or reducing count). That will resolve the issue, but change current behavior:
> > 
> >   if (condition1) {
> >     if (condition) {
> >       UP = ...
> >     } else {
> >       UP.Count = 1;
> >       Warn();
> >     }
> >     return true;
> >   }
> > 
> > Now it is very annoying when I tell compiler to unroll by 8, but it unrolls by 4 just because of some heuristics without any warnings.
> > 
> Personally, I like what you suggested as well. I think when pragmas are used, the user wants full control over what happens and currently we don't provide that. Also, I really like that the code will be much simpler in this case.
> 
> Others might have different views on that though.
Anyway to stress current logic and ask for changes the code need to be refactored first.


Repository:
  rL LLVM

http://reviews.llvm.org/D19553