[PATCH] D41766: [MachineCombiner] Add check for optimal pattern order.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: lib/CodeGen/MachineCombiner.cpp:485
 
+#ifdef EXPENSIVE_CHECKS
+    // Check that the difference between original and new latency is
----------------
Gerolf wrote:
> Could that go into a separate function? You might want to control it by an internal flag rather than a compile-time define.
Done! The option is enabled with expensive checks. I've also enabled the option in a couple of machine-combiner tests.


================
Comment at: lib/CodeGen/MachineCombiner.cpp:506
+      long CurrentLatencyDiff = ((long)RootLatency) - ((long)NewRootLatency);
+      assert(CurrentLatencyDiff <= PrevLatencyDiff &&
+             "Current pattern is better than previous pattern.");
----------------
Gerolf wrote:
> You might want to add a dumping capability that prints the pattern in a sorted order. Did you encounter scenarios where different orderings of pattern (other than the default) gave improvements in some cases, but not others? If these cases can be tight to some characteristic in the program it might be possible to pass this information on to getMachineCombinerPatterns() and return a different order.
I could add dumping of the sorted patterns, but I think it would be better to  do it in a separate commit. 

I did not find any cases where a different order gave improvements in some cases. Although it would be worth to run this verification on larger benchmarks (e.g. SPEC2017), which should be easy with the new option.


https://reviews.llvm.org/D41766