[PATCH] D11758: [Unroll] Implement a conservative and monotonically increasing cost tracking system during the full unroll heuristic analysis that avoids counting any instruction cost until that instruction becomes "live" through a side-effect or use outside the...

[Michael Zolotukhin via llvm-commits]

mzolotukhin marked 14 inline comments as done.
mzolotukhin added a comment.

Hi,

I committed the patch in r269388. Along with the changes you requested I adjusted some thresholds in tests invocation and made one semantical change in visitPHINode. In the previous version we checked `if (PN.getParent() == L->getHeader())` and early exited if it's true. That prevented us from running `simplifyWithSCEV` on the phi-value, so the analysis didn't get the information about this phi. Now we run the base visitor first to mine the data if we can, and only then we check if the phi is in the header.

Michael


================
Comment at: lib/Transforms/LoopUnroll/full-unroll-heuristics-dce.ll:1
@@ +1,2 @@
+; RUN: opt < %s -S -loop-unroll -unroll-max-iteration-count-to-analyze=100 -unroll-dynamic-cost-savings-discount=1000 -unroll-threshold=10 -unroll-percent-dynamic-cost-saved-threshold=80 | FileCheck %s
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
----------------
chandlerc wrote:
> This file is under lib, not test?
Ouch, thanks for the catch!

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:241-244
@@ -206,1 +240,6 @@
 
+  // Only analyze inner loops. We can't properly estimate cost of nested loops
+  // and we won't visit inner loops again anyway.
+  if (!L->empty())
+    return None;
+
----------------
chandlerc wrote:
> It would make slightly more sense to hoist this out to the caller... There is nothing about this routine that is specific to inner loops, its just that it isn't useful right now?
> 
> Not a big deal either way.
We need to treat inner loops in a special way (e.g. using weights for basic blocks based on the profile info), but we're not doing it now, so I tend to think this function in current implementation isn't designed to work on outer loops.

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:270
@@ +269,3 @@
+  // we don't count the cost of any dead code. This is essentially a map from
+  // <instrution, int> to <bool, bool>, but stored as a densely packed struct.
+  DenseSet<UnrolledInstState, UnrolledInstStateKeyInfo> InstCostMap;
----------------
chandlerc wrote:
> Heh, now that you've taken this over, need to address your own comment here. ;]
Fixed :)

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:417
@@ +416,3 @@
+        // unrolling and continue.
+        bool IsFree = Analyzer.visit(I);
+        bool Inserted = InstCostMap.insert({&I, (int)Iteration, IsFree,
----------------
chandlerc wrote:
> I have no problem adding them back. Want to do it in a follow-up patch? In this patch?
I'll add them back in another patch if I see a need in them.

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:430-431
@@ +429,4 @@
+        if (I.mayHaveSideEffects())
+          AddCostRecursively(I, Iteration);
+
+        // Can't properly modeal a cost of a call.
----------------
chandlerc wrote:
> Not sure what this comment was referencing...
> 
> The idea is that all out-of-loop uses should be via a LCSSA phi node in the exit block(s) and we backwards saturate their cost there?
Yeah, I think I might misunderstand this part at that time.


Repository:
  rL LLVM

http://reviews.llvm.org/D11758