[PATCH] D63809: [HardwareLoops] Loop counter guard Loop intrinsic
Sjoerd Meijer via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 27 05:31:00 PDT 2019
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
More information about the llvm-commits
mailing list