[PATCH] D23388: [LoopUnroll] By default disable unrolling when optimizing for size.

[Michael Zolotukhin via llvm-commits]

mzolotukhin added inline comments.

================
Comment at: include/llvm/CodeGen/BasicTTIImpl.h:284-288
@@ -283,3 +283,7 @@
     UP.Partial = UP.Runtime = true;
-    UP.PartialThreshold = UP.PartialOptSizeThreshold = MaxOps;
+    UP.PartialThreshold = MaxOps;
+
+    // Avoid unrolling when optimizing for size.
+    UP.OptSizeThreshold = 0;
+    UP.PartialOptSizeThreshold = 0;
   }
----------------
chandlerc wrote:
> So this is just designed to fairly closely replicate the behavior before we put the pass into the pipeline?
> 
> It'd be good to have some kind of FIXME or whatever. Much like inlining, I would expect that sufficiently conservative unrolling will still be a win in opt-size mode.
> So this is just designed to fairly closely replicate the behavior before we put the pass into the pipeline?
For now - basically yes.

> It'd be good to have some kind of FIXME or whatever. Much like inlining, I would expect that sufficiently conservative unrolling will still be a win in opt-size mode.
Also, it's only a generic implementation, targets are free to override it with other values.

In general I agree that on Os (in contrast to Oz) we should allow some unrolling. But I prefer to be extra careful here not to do more harm than good - for now I'm just reproducing what we did before the change, while keeping the way clear for enabling it later.

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:951-953
@@ -950,1 +950,5 @@
 
+  // Exit early if unrolling is disabled.
+  if (UP.Threshold == 0 && UP.PartialThreshold == 0)
+    return false;
+
----------------
chandlerc wrote:
> Generally this makes sense, but if we're not doing partial unrolling, but we have a non-zero partial threshold, we won't early-exit even though threshold is zero... Maybe structure the checks so we only consider the partial threshold when considering partial unrolling?
This makes sense.


https://reviews.llvm.org/D23388