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

mzolotukhin added a comment.

Hi Chandler,

Mostly the patch looks good, a couple of nit-picks inline.

But I think we need more tests for this. I think we can add debug prints and pin-point the cases we want to check with it in tests.

Thanks,
Michael


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:596
@@ +595,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;
----------------
s/instrution/instruction/

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:616
@@ +615,3 @@
+
+        auto CostIter = InstCostMap.find({I, Iteration, 0, 0});
+        if (CostIter == InstCostMap.end())
----------------
Maybe worth commenting here that only `I` and `Iteration` are actually used as a key. It might be not-obvious for someone looking at this code without context of this patch.

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:620
@@ +619,3 @@
+          // we may have no cost data for it here. What that actually means is
+          // that it is free.a
+          continue;
----------------
s/free.a/free./

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:739
@@ +738,3 @@
+        // unrolling and continue.
+        bool IsFree = Analyzer.visit(I);
+        bool Inserted = InstCostMap.insert({&I, (int)Iteration, IsFree,
----------------
I find having debug prints very convenient in this kind of analyses. Would it be possible to return them back?

We could report instructions that was simplified, and instructions that were proven to be live, for instance. Also, I think we can use this diagnostic in tests, as it gets harder and harder to write them.

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:749
@@ -616,4 +748,3 @@
 
-        // Also track this instructions expected cost when executing the rolled
-        // loop form.
-        RolledDynamicCost += InstCost;
+        // If the instruction is observable either through a side-effect
+        // recursively account for the cost of it and all the instructions
----------------
Is word 'either' redundant here, or is it just my English?:)


http://reviews.llvm.org/D11758