[PATCH] D63809: [HardwareLoops] Loop counter guard Loop intrinsic

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: lib/CodeGen/HardwareLoops.cpp:284
+  // loop counter and tests that is not zero?
+  auto CanGenerateTest = [](Loop *L, Value *Count) {
+    BasicBlock *Preheader = L->getLoopPreheader();
----------------
A nit, and more a style preference, so please ignore if you disagree! 
The first thing in `initLoopCount` is a very big lambda definition. But I think `CanGenerateTest` would be a good candidate for a local helper function as it requires very few arguments and only returns a bool, so that InitLoopCount becomes smaller and easier to read.


================
Comment at: lib/CodeGen/HardwareLoops.cpp:303
+
+    if (auto *Const = dyn_cast<ConstantInt>(ICmp->getOperand(0))) {
+      if (!Const->isZero())
----------------
Another nit: I think this could be nice candidate for a lambda to share code, i.e. the checks are the same in the if-clause and else-if clause, just the indexes are swapped.


================
Comment at: lib/CodeGen/HardwareLoops.cpp:329
+  UseLoopGuard |= ForceGuardLoopEntry;
+  if (UseLoopGuard && BB->getSinglePredecessor() &&
+      cast<BranchInst>(BB->getTerminator())->isUnconditional())
----------------
I am now actually wondering if we are checking here if the loop is particular kind of loop, i.e. a Single-Entry Single-Exit loop? Or, do we e.g. get this 'guarantee' from SCEV, because otherwise we can't calculate an iteration count? But what happens here if there are multiple predecessors?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63809/new/

https://reviews.llvm.org/D63809