[PATCH] D13417: [MachineCombiner] make slack optional in critical path cost calculation (PR25016)

[Sanjay Patel via llvm-commits]

spatel added a reviewer: stoklund.
spatel added a comment.

In http://reviews.llvm.org/D13417#260911, @spatel wrote:

> On 2nd thought, that slack looks really wrong, so maybe the right fix for this problem actually is in MachineTraceMetrics?


On 3rd thought, that looks fine. I misunderstood the meanings of 'height' and 'depth' in MTM.
So I've returned to the conclusion that the bug is in MachineCombiner's handling of slack. If you examine how slack is used in EarlyIfConversion:
http://llvm.org/docs/doxygen/html/EarlyIfConversion_8cpp_source.html#l00729

I think that shows the intended usage: slack is limited both by a dynamic calculation and a heuristic. Without those limits, we're almost certain to increase the critical path and lose performance (as shown in the examples here).

I don't understand the concern with using different cost heuristics per pattern. We recalculate all of the trace metrics after performing a transform, so there should be no interference between transforms. Making reassociation have a more conservative cost equation than AArch64's MADD transform seems natural to me because the transforms have different characteristics.

For example, I see a further compile-time optimization / differentiation based on patterns that we can use here (in a follow-on patch of course): by definition, reassociation will produce the same number and types of instructions, so we don't need to calculate MTM 'heights' (resource usage must be identical). Calculating the critical path via 'depth' (data dependency chains) should be enough to decide if a reassociation transform will help.


http://reviews.llvm.org/D13417