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

Hi Chandler,

I found that with this patch compile time is significantly worse on some benchmarks, a reduced testcase from one of them is attached. It seems like the problem isn’t in the patch itself, but in the way we perform actual unrolling - you can see that if you add ‘-debug’ flag. I can look into what’s bad there later, but if you’d like to poke it too, you are welcome:)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: reduced.ll
Type: application/octet-stream
Size: 2299 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150805/9db1ede0/attachment.obj>
-------------- next part --------------


Interestingly, compile-time in this case seems to be cubic on number of iterations of the outer loop.

Thanks,
Michael

> On Aug 5, 2015, at 3:23 PM, Michael Zolotukhin <mzolotukhin at apple.com> wrote:
> 
> 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
> 
> 
>