[PATCH] D21720: Unroll for uncountable loops

[Eli Friedman via Phabricator via llvm-commits]

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