[PATCH] D30941: Better testing of schedule model instruction latencies/throughputs

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D30941#706295, @avt77 wrote:

> Hal,
>  I removed the special option (-print-schedule) and tried to check-all. The result was very unpleseant but predictable:
>
>   Expected Passes    : 29409
>   Expected Failures  : 160
>   Unsupported Tests  : 442
>   Unexpected Failures: 643
>   
>
> Should I fix all these 643 failures or it's better to keep the option?


No. Mostly because it is not entirely mechanical (it does not make sense to update the tests without understanding whether the results make sense; the person who updates the test should hopefully have that background). FWIW, this also does not make sense for higher-level targets (e.g. NVPTX).

I recommend the following:

1. Add a target callback to enable the printing of the scheduling information during verbose mode
2. Provide a command-line option (cl::opt) that can override that target callback (for easy testing), as in:

  bool Enable = OptionName.getNumOccurrences() ? OptionName : STI->shouldPrintStuff();
3. Enable this only on x86 (fix those tests, if any) (*)

Once we think this works reasonably well, we can encourage other backend maintainers to look at updating their tests and enabling it on their targets if they'd like.

(*) Given that FileCheck does prefix matching, and we're adding this information last on the line, I'm curious to understand why the tests are matching that is causing these failures.



================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:766
+      else if (Latency > 0)
+        CommentOS << "[" << Latency << ":???]\n";
+      else if (RThroughput != std::numeric_limits<double>::infinity())
----------------
I think that one ? is enough (three looks excessive to me).


================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:768
+      else if (RThroughput != std::numeric_limits<double>::infinity())
+        CommentOS << "[???:" << RThroughput << "]\n";
+    }
----------------
One only ? here please.


https://reviews.llvm.org/D30941