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

[Chandler Carruth via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

Seems fine to mitigate compile time issues, but I think we need some kind of opt-size loop unrolling test cases that check we *do* unroll obviously benificial loops under optsize and we *don't* unroll obvious bad loops that would have been unrolled in a non-optsize function. LGTM, but whether in this commit or a follow-up, I'd like to get at least obvious cases here covered by tests.


================
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;
   }
----------------
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.

================
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;
+
----------------
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?


https://reviews.llvm.org/D23388