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

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
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;
----------------
fhahn wrote:
> 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.
Yep, I missed a test, thanks for catching.

I wanted them grouped since the full unroll does use the estimator along one path.  It felt "odd" to have some looks fully unrolled on uniform, and some not.  No strong preference though.


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:738
+///      if (a[i/4])
+///        b[i++;
+///    }
----------------
xbolva00 wrote:
> Typo
Will fix, thanks.


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:806
 
+  LoopSizeEstimator LSE(*L, SE, LoopSize);
+
----------------
fhahn wrote:
> 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?
Can do.  Need to do that for the follow on, I just tried to keep this one simple.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91481