[PATCH] D28897: [PM] Simplify the new PM interface to the loop unroller and expose two factory functions for the two modes the loop unroller is actually used in in-tree: simplified full-unrolling and the entire thing including partial unrolling.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D28897#654378, @davide wrote:

> I have a fundamental question about the constructors.


See updated patch! =D

> Also, I would prefer the spelling `FullUnroll` instead of `SimpleUnroll`.

Not sure what you actually want yet...

Currently we have `LoopUnrollPass::create()` and `LoopUnrollPass::createSimple()`, and at the pipeline `unroll` and `unroll-simple`. Are you suggesting just using "full" instead of "simple" with the same structure? `unroll-full` makes sense, but `LoopUnrollPass::createFull()` seems weird. It is a less powerful pass...



================
Comment at: include/llvm/Transforms/Scalar/LoopUnrollPass.h:25-31
+public:
+  static LoopUnrollPass createSimple() {
+    return LoopUnrollPass(/*AllowPartialUnrolling*/ false);
+  }
+  static LoopUnrollPass create() {
+    return LoopUnrollPass(/*AllowPartialUnrolling*/ false);
+  }
----------------
chandlerc wrote:
> davide wrote:
> > Is it intended that you're passing `false` to both `create()` and `createSimple()` ?
> > Shouldn't `create()` allow the entire thing, including partial unrolling? Here you don't enable it at all.
> Ahem. Indeed, that was not intended. More tests are also in order here!
> 
> (This isn't heavily used yet, so it isn't that surprising that i've missed a test for this.)
It was worse than you realize.

The flag was actually backwards so that, combined with this being false, made both of these the more generic unroller.

I've added a test that the simple unroller *just* does full unrolling and that this one *does* do partial unrolling. Sorry for getting all my flags wrong.


https://reviews.llvm.org/D28897