[PATCH] D39728: Add -print-schedule scheduling comments to inline asm

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Overall, the patch looks good to me.
I made some minor comments below.



================
Comment at: include/llvm/MC/MCParser/MCAsmParser.h:131
   bool HadError = false;
+  /// Enable print [latency:throughput] in output file
+  bool EnablePrintSchedInfo = false;
----------------
Comments should end with a period. I noticed the same mistake in AsmPrinter.h.


================
Comment at: include/llvm/MC/MCParser/MCAsmParser.h:135
   SmallVector<MCPendingError, 1> PendingErrors;
   /// Flag tracking whether any errors have been encountered.
 
----------------
Unrelated to your code review. However, this comment should be associated to field 'HadError' at line 130. I think this should be fixed.


================
Comment at: include/llvm/MC/MCParser/MCAsmParser.h:169
+  void setEnablePrintSchedInfo(bool Value) { EnablePrintSchedInfo = Value; }
+  bool enablePrintSchedInfo() { return EnablePrintSchedInfo; }
+
----------------
I suggest to rename `enablePrintSchedInfo()` to `shouldPrintSchedInfo()`.


================
Comment at: lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp:198
+                                    MCContext &Ctx, const MCInstrInfo &MII,
+                                    MCStreamer &Out, bool) override {
     InstrumentMOVS(Inst, Operands, Ctx, MII, Out);
----------------
Add a `/* unused */` for the bool param.


================
Comment at: lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp:1046-1047
     const MCInst &Inst, OperandVector &Operands, MCContext &Ctx,
-    const MCInstrInfo &MII, MCStreamer &Out) {
-  EmitInstruction(Out, Inst);
+    const MCInstrInfo &MII, MCStreamer &Out, bool EnableSchedInfo) {
+  EmitInstruction(Out, Inst, EnableSchedInfo);
 }
----------------
I suggest to change `EnableSchedInfo` to just `PrintSchedInfo` (or `PrintSchedInfoEnabled`).


================
Comment at: lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp:1050-1052
+void X86AsmInstrumentation::EmitInstruction(MCStreamer &Out, const MCInst &Inst,
+                                            bool EnableSchedInfo) {
+  Out.EmitInstruction(Inst, *STI, EnableSchedInfo);
----------------
Same.


================
Comment at: lib/Target/X86/AsmParser/X86AsmInstrumentation.h:45-46
       SmallVectorImpl<std::unique_ptr<MCParsedAsmOperand>> &Operands,
-      MCContext &Ctx, const MCInstrInfo &MII, MCStreamer &Out);
+      MCContext &Ctx, const MCInstrInfo &MII, MCStreamer &Out,
+      bool EnableSchedInfo);
 
----------------
I suggest to change `EnableSchedInfo` to just `PrintSchedInfo` (or `PrintSchedInfoEnabled`).


================
Comment at: lib/Target/X86/AsmParser/X86AsmInstrumentation.h:58-59
 
-  void EmitInstruction(MCStreamer &Out, const MCInst &Inst);
+  void EmitInstruction(MCStreamer &Out, const MCInst &Inst,
+                       bool EnableSchedInfo = false);
 
----------------
Same.


================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:2704
+      Inst, Operands, getContext(), MII, Out,
+      getParser().enablePrintSchedInfo());
 }
----------------
I suggest to rename `enablePrintSchedInfo()` to `shouldPrintSchedInfo()`.


https://reviews.llvm.org/D39728