[PATCH] D27004: Set unroll remainder to epilog if profitable

[Michael Zolotukhin via Phabricator via llvm-commits]

mzolotukhin added inline comments.


================
Comment at: test/Transforms/LoopUnroll/epilog_const_phi.ll:4
+; Epilog unroll allows to keep PHI constant value.
+; For the test this means that inscombine can completle eliminate XOR.
+
----------------
evstupac wrote:
> mzolotukhin wrote:
> > s/inscombine/instcombine/
> > s/completle/completely/
> > 
> > I'd rather not rely on other passes (instcombine) to check loop-unroll. Can we check the CFG instead? That'll make the test smaller (since it'll be enough to have a minimal loop) and more robust to unrelated changes (say, instcombine for some reason stopped to simplify XOR through phis). It would be also nice to have a test, in which we don't use epilog (i.e. arguments of phis are not constant), to check that we're not doing it everywhere.
> >I'd rather not rely on other passes (instcombine) to check loop-unroll. Can we check the CFG instead? That'll make the test smaller (since it'll be enough to have a minimal loop) and more robust to unrelated changes (say, instcombine for some reason stopped to simplify XOR through phis). 
> I agree, that right now it is more than unroll test. We can switch to CFG, but I'd like to leave XOR or smth else that shows clear profitability (when other optimizations are applied) instead of minimizing the loop.
> 
> >It would be also nice to have a test, in which we don't use epilog (i.e. arguments of phis are not constant), to check that we're not doing it everywhere.
> Good point. I'll add such.
> 
> I agree, that right now it is more than unroll test. We can switch to CFG, but I'd like to leave XOR or smth else that shows clear profitability (when other optimizations are applied) instead of minimizing the loop.
The goal of the tests is to test stuff, not to show profitability:) I'd rather mention the profitability in comments (which I think you already did). I think the smaller (~more decoupled) test the better.

> I'll add such.
Thanks. Feel free to commit after it, no need to repost the updated patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D27004