[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...

[Chandler Carruth via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

OK, I've paged all this back into my brain.

The inner loop aspect makes more sense to me now. I'm still a bit dubious on the skipping calls, but I understand that at least we're not ready for them yet and its important to get this stuff moving, so it seems like a good incremental step.

Some comments below. Only the test for the inner loop case is really interesting. Feel free to submit with these comments addressed or post any updates with more questions if useful.


================
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"
----------------
This file is under lib, not test?

================
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;
+
----------------
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.

================
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;
----------------
Heh, now that you've taken this over, need to address your own comment here. ;]

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:290
@@ +289,3 @@
+
+        auto CostIter = InstCostMap.find({I, Iteration, 0, 0});
+        if (CostIter == InstCostMap.end())
----------------
I'm fine with a comment, or extending the DenseMapInfo to support using .find_as(std::make_pair(I, Iteration)).

================
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,
----------------
I have no problem adding them back. Want to do it in a follow-up patch? In this patch?

================
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.
----------------
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?

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:432-433
@@ +431,4 @@
+
+        // Can't properly modeal a cost of a call.
+        if(isa<CallInst>(&I))
+          return None;
----------------
Maybe leave a FIXME? I feel like we should make some effort to address this in follow-up commits. At the very least I suspect we want to handle intrinsics here.


http://reviews.llvm.org/D11758