[PATCH] D91481: [LoopUnroll] Discount uniform instructions in cost models

Sat Nov 14 11:56:07 PST 2020

fhahn added a comment.

> Reviewers, I am not entirely sure this is the right approach. The obvious alternative is to generalize the full unrolling cost model to handle partial unrolling. As detailed below, the alternative looks non-trivial, and I decided against it. I do see the argument for that being the right approach though, so if you want to just reject this patch outright, I won't argue.

Agreed, this is going to be tricky, as the full unrolling cost-model tries hard to estimate what can be eliminated through full unrolling, which would need a lot of adjustments for partial unrolling. Moving forward with improving them separately seems reasonable to me for now.

================
Comment at: llvm/lib/Analysis/LoopUnrollAnalyzer.cpp:40
+  // Given that, all but the first occurance are free.
+  if (!IterationNumber->isZero() && SE.isLoopInvariant(S, L))
+    return true;
----------------
Unless I am missing something, It seems like there's a test case missing for this change? Would it be possible to add one?

This could also be split into 2 separate patches, to make it slightly easier to pin-point potential regressions, but I am not sure if that's worth it.

================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:806

+  LoopSizeEstimator LSE(*L, SE, LoopSize);
+
----------------
LoopSizeEstimator iterates over the whole loop body on construction, but it might not be used. Would it be possible to change it to compute the size on demand and cache it for further accesses?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91481