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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

A number of minor style comments.

@hfinkel does this look alright to you as a patch for initial support for scheduler comments?



================
Comment at: include/llvm/CodeGen/AsmPrinter.h:119
+  TargetSchedModel SchedModel;
+  /// Enable print [latency:throughput] in output
+  bool EnablePrintSchedInfo = false;
----------------
style - newline before comment, add '.' to end of comment.


================
Comment at: include/llvm/CodeGen/TargetSchedule.h:184
                                 const MachineInstr *DepMI) const;
+  double computeInstrRThroughput(const MachineInstr *MI,
+                                   bool UseDefaultThroughput = true) const;
----------------
RKSimon wrote:
> Add doxygen comment.
newline


================
Comment at: include/llvm/Target/TargetSubtargetInfo.h:22
 #include "llvm/CodeGen/ScheduleDAGMutation.h"
+#include "llvm/CodeGen/SchedulerRegistry.h"
+#include "llvm/MC/MCInst.h"
----------------
Any clang-format / include 'juggling' should be done as a NFC cleanup commit, not as part of a larger patch.


================
Comment at: include/llvm/Target/TargetSubtargetInfo.h:147
 
+  /// \brief Enable print [latency:throughput] comment in output .S file
+  virtual bool supportPrintSchedInfo() const { return false; }
----------------
Comment says 'enable' but function says 'support' - which is it? Full stop at end of comment (style)


================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:772
+
+  if (Commented && AP->EnablePrintSchedInfo)
+    CommentOS << " " << MF->getSubtarget().getSchedInfoStr(MI) << "\n";
----------------
Add brief comment explaining what you're doing.


================
Comment at: lib/CodeGen/TargetSchedule.cpp:281
+  if (SCDesc->isValid())
+    llvm_unreachable("No MI sched latency: SCDesc->isVariant()");
+  // TODO: some opcodes don't have valid MCSchedClassDesc?
----------------
Shouldn't this be an assert?


================
Comment at: lib/MC/MCAsmStreamer.cpp:106
   /// AddEncodingComment - Add a comment showing the encoding of an instruction.
-  void AddEncodingComment(const MCInst &Inst, const MCSubtargetInfo &);
+  void AddEncodingComment(const MCInst &Inst, const MCSubtargetInfo &,
+                          bool PrintSchedInfo);
----------------
Explain how PrintSchedInfo will be used in comment


================
Comment at: lib/MC/MCAsmStreamer.cpp:1584
+  OS << "]";
+  if (Fixups.size() || !PrintSchedInfo)
+    OS << "\n";
----------------
Comment this


================
Comment at: lib/MC/MCObjectStreamer.cpp:231
 void MCObjectStreamer::EmitInstruction(const MCInst &Inst,
-                                       const MCSubtargetInfo &STI) {
+                                       const MCSubtargetInfo &STI, bool) {
   MCStreamer::EmitInstruction(Inst, STI);
----------------
You're inconsistent leaving some unused argument without names and other with.


================
Comment at: lib/Object/RecordStreamer.cpp:81
 void RecordStreamer::EmitInstruction(const MCInst &Inst,
-                                     const MCSubtargetInfo &STI) {
-  MCStreamer::EmitInstruction(Inst, STI);
+                                     const MCSubtargetInfo &STI, bool B) {
+  MCStreamer::EmitInstruction(Inst, STI, B);
----------------
'B' is meaningless - please use a real name to explain its usage.


================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFStreamer.h:49
+  void EmitInstruction(const MCInst &Inst, const MCSubtargetInfo &STI,
+                       bool B = false) override;
 
----------------
Why have you include a default value here??


================
Comment at: lib/Target/X86/InstPrinter/X86InstComments.h:26
   class raw_ostream;
+  class AsmPrinter;
   bool EmitAnyX86InstComments(const MCInst *MI, raw_ostream &OS,
----------------
Should this be here or inside the cpp file(s) ?


================
Comment at: lib/Target/X86/X86Subtarget.cpp:378
+
+static char *SchedPrefix = " sched: [";
+static std::string creatSchedInfoStr(unsigned Latency,
----------------
Probably best to bring this inside creatSchedInfoStr?


================
Comment at: lib/Target/X86/X86Subtarget.cpp:379
+static char *SchedPrefix = " sched: [";
+static std::string creatSchedInfoStr(unsigned Latency,
+                                     Optional<double> RThroughput) {
----------------
typo: createSchedInfoStr


================
Comment at: lib/Target/X86/X86Subtarget.h:632
+  TargetSchedModel TSchedModel;
+  /// Returns string representation of scheduler comment.
+  virtual std::string getSchedInfoStr(const MachineInstr &MI) const override;
----------------
newline


https://reviews.llvm.org/D30941