[PATCH] D21720: Unroll for uncountable loops
Eli Friedman via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 14 15:32:25 PDT 2017
efriedma added a comment.
> Changing it in this patch would be considered as "unrelated change".
It's still messy, and still needs to change before this lands. Changing it in a separate patch is fine, if that makes sense.
----
This change needs testcases, including a couple examples of loops where the unrolling is profitable.
================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:413
+ OpIP->getIncomingValueForBlock(L->getLoopLatch())))
+ CostWorklist.push_back(OpIPI);
+ }
----------------
Indentation. And I'm not sure why you're adding an instruction to the worklist that might not even be in the loop.
================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:466
+
+ unsigned PhiCycle = getPhiCycleLength (PHI, L);
+ // For complete unroll if we were unable to get PHI cycle length
----------------
Maybe this would be more clear if it returned a boolean? I'm not sure the rest of this code makes sense if PhiCycle isn't either 0 or 2.
================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:608
+ if (OpPN->getParent() == L->getHeader())
+ Op = OpPN->getIncomingValueForBlock(L->getLoopLatch());
if (auto *OpI = dyn_cast<Instruction>(Op))
----------------
What is this change doing? (Please put comments in the code.)
================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:987
+ if (!Cost || Cost->UnrolledCost >= Cost->RolledDynamicCost ||
+ (unsigned)Cost->UnrolledCost > UP.PartialThreshold)
+ UP.Force = false;
----------------
"Cost->UnrolledCost >= Cost->RolledDynamicCost" is the profitability check? Needs a comment to explain what that means.
Do we care how large the improvement is vs. the size of the loop?
================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:990
+ }
+ if (UP.Force) {
+ // Currently force unroll only by 2.
----------------
Maybe rearrange this? UP.Force is only true on one codepath out of the previous if statement.
Repository:
rL LLVM
https://reviews.llvm.org/D21720
More information about the llvm-commits
mailing list