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

[Michael Zolotukhin via Phabricator via llvm-commits]

mzolotukhin added a comment.

Hi Evgeny,

Please find some comments inline.

Thanks



================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:207
+  BasicBlock *Header = L->getHeader();
+  assert(Latch || PreHeader || Header);
+  for (Instruction &BBI : *Header) {
----------------
1) `Latch`is not used anywhere.
2) `&&` should be used instead of `||`.


================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:212-213
+      break;
+    if (PN->getBasicBlockIndex(PreHeader) < 0)
+      continue;
+    Value *V = PN->getIncomingValueForBlock(PreHeader);
----------------
This shouldn't be possible - I think we can assert this (if `getIncomingValueForBlock` doesn't assert it already).


================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:215-216
+    Value *V = PN->getIncomingValueForBlock(PreHeader);
+    if (!V)
+      continue;
+    if (ConstantInt *CI = dyn_cast<ConstantInt>(V)) {
----------------
This also should not be possible, so we can replace it with assert.


================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:221-225
+      if (Val.getBitWidth() <= 31 && (1UL << Val.getBitWidth()) <= Count)
+        return true;
+      APInt Divider (Val.getBitWidth(), (uint64_t)Count);
+      if (Val.urem(Divider) == 0)
+        return true;
----------------
Where is this come from? Can you please elaborate what we're doing here?


================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:360
       });
+  bool EpiplogProfitability =
+      UnrollRuntimeEpilog.getNumOccurrences() ? UnrollRuntimeEpilog
----------------
s/Epiplog/Epilog/


================
Comment at: test/Transforms/LoopUnroll/unroll-pragmas.ll:181-184
+; CHECK: for.body.epil:
+; CHECK: store
+; CHECK-NOT: store
+; CHECK: br i1
----------------
Can we also have a separate test for the new functionality?


Repository:
  rL LLVM

https://reviews.llvm.org/D27004