[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#707464, @avt77 wrote:

> I did everything accordingly to Hal's requirements except one: the default value of "print-schedule" switch is false because otherewise we have "Unexpected Failures: 530" and it's X86 tests ony. The problem is very simple: update_llc_test_checks.py generates CHECKs like here
>
> ; XOP-AVX1-NEXT:    vextractf128 $1, %ymm2, %xmm5


I did not realize that CHECK-NEXT always matched the whole line. That's interesting.

In any case, if this is an update_llc_test_checks.py problem, why don't you use it to update the tests?

> It means FileCheck has to check the whole line but this patch adds the comment at the end of line and as result the line can't be checked properly.
>  Finaly what we have now:
> 
>   - we have option "-print-schedule" allowing to print [latency:throughput" in output (default is false)
>   - we have  enablePrintSchedInfo() - default is false
> - X86 overrides  enablePrintSchedInfo()
> 
>   Hope that's exactly what was required.



================
Comment at: include/llvm/CodeGen/AsmPrinter.h:120
+  /// Enable print [latency:throughput] in output .S
+  bool enablePrintSchedInfo = false;
+
----------------
Variable names should start with a capital letter:  EnablePrintSchedInfo


================
Comment at: include/llvm/MC/MCTargetOptions.h:50
   bool AsmVerbose : 1;
+  bool PrintSchedule : 1;
 
----------------
Remove this option.


================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1398
   ORE = &getAnalysis<MachineOptimizationRemarkEmitterPass>().getORE();
-  if (isVerbose())
+  if (isVerbose()) {
     LI = &getAnalysis<MachineLoopInfo>();
----------------
We don't add { } around single-statement blocks.


================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1400
     LI = &getAnalysis<MachineLoopInfo>();
+  }
+  if (TM.Options.MCOptions.PrintSchedule &&
----------------
When you remove the '}' here, I'd keep the blank line ;)


================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1401
+  }
+  if (TM.Options.MCOptions.PrintSchedule &&
+        MF.getSubtarget().enablePrintSchedInfo()) {
----------------
The command-line option should be declared in this file. That way you can predicate using the command-line option value on getNumOccurrences() as I had indicated previously. Plus, there's no need to have the option name depend on the tool.

Full disclosure: There is another option: You can use a tristate setup, look for DefaultOnOff in lib/CodeGen/AsmPrinter/DwarfDebug.cpp.



================
Comment at: lib/CodeGen/TargetSchedule.cpp:315
+                                          bool UseDefaultRThroughput) const {
+  if (UseDefaultRThroughput) {
+    // TODO: we should arrange something like we have on http://www.agner.org/
----------------
Please remove this comment and the UseDefaultRThroughput variable. We can always add it later if we'd like.


================
Comment at: lib/CodeGen/TargetSchedule.cpp:321
+  if (!hasInstrSchedModel() && !hasInstrItineraries())
+    return Unknown;
+
----------------
This Unknown value should never be returned (this is an implementation detail). Please return an Optional<double>.


https://reviews.llvm.org/D30941