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

[Gerolf Hoflehner via llvm-commits]

Gerolf added a comment.

I think we need your patch. It would be nice to have also a test case that shows an actual increase in critical path length and a measurable performance difference. In the test cases so far the vadd sequences take longer w/o actually increasing the critical path. So just by looking at a reassociation case in isolation I wouldn’t worry about modifying the heuristic. However, in a scenario eg. with reassoc first and then a mul+add -> madd the bigger depth of the re-associated instruction could result in not performing the mul+ add combine later. It could be that the extra depth may have eaten the slack the madd needs to fit within the critical path.

Also, since reassoc attempts to reduce depth shouldn’t the equation ignore latencies and the code finally look something like if (MustReduceDepth) return NewDepth < OldDepth else if (MustReduceCPL) return NewCycleCount < OldCycleCount else return NewCycleCount <= OldCycleCount?

Thanks for effort and being persistent.

Cheers
Gerolf


http://reviews.llvm.org/D13417