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

[Sanjay Patel via llvm-commits]

spatel added a comment.

In http://reviews.llvm.org/D13417#285442, @Gerolf wrote:

> Now that you check for schedule depth for reassociations do you still need to check for shorter length? If so, for which pattern? I think we should get away with checks for depth (MustReduceDepth) and just leave the default (don't prolong critical path) for the muladd etc patterns.


Good catch: we were only distinguishing between < and <= to prevent reassociations from firing inadvertently. We can make that always <= now for the default (muladd) patterns. Note that there are no muladd regression tests that actually check for that difference though.


================
Comment at: lib/CodeGen/MachineCombiner.cpp:329
@@ -321,1 +328,3 @@
 
+/// For some patterns, we want to use a more conservative cost metric when
+/// when evaluating if a transform is beneficial. These patterns require that
----------------
Gerolf wrote:
> could we have that function return eg. an enum CombinerObjective that return MustReduceDepth or Default? Then improvesCriticalPathLength only needs one parameter of that type.
Sure - I think we can pass the pattern to improvesCriticalPathLength, and then it can unwrap whatever properties it needs internally. This makes the C++ enum class deficiency even more obvious. We could wrap the enums in a class, but that should be a separate change IMO.


http://reviews.llvm.org/D13417