Fix for bug PR35549 - [X86] Repeated schedule comments

[avt77]

Summary

I fixed the issue.

[RKSimon]

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

745–747

update the comment to explain what the return value means

795

just return true here? Then you can remove Result entirely and return false at the end of the function.

1044

if (isVerbose() && emitComments(MI, OutStreamer->GetCommentOS(), this)) {

It'd be much safer to add a MachineInstr::setSchedInfoFlags(bool) style helper and avoid the hard coded constant here.

The whole block needs to be passed through clang-format as well.

lib/Target/X86/MCTargetDesc/X86BaseInfo.h

63

Add a comment describing the flag as its different from the other flags which are about prefixes.

lib/Target/X86/X86MCInstLower.cpp

107

Again, get rid of this hard coded constant

2008

Won't this copy other flags as well? Is that safe?

[avt77]

lib/Target/X86/X86MCInstLower.cpp

2008

Yes, that's exactly what we need here.

[RKSimon]

lib/Target/X86/X86MCInstLower.cpp

2008

So is this a separate issue that needs tests and possibly a separate preliminary patch? Are you missing prefix flags data because of this?

[avt77]

lib/Target/X86/X86MCInstLower.cpp

2008

I was completely wrong here: I mixed Flags from MCInst and MachineInstr. It's redone now.

[avt77]

All comments from Simon were resolved. There was a real bug and it's fixed in this new patch - tnx, Simon.

[RKSimon]

include/llvm/CodeGen/MachineInstr.h

73

The TAsmComments/AC_EVEX_2_VEX hard-coded enum fix should be done as a separate pre-commit.

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

796

You can probably either replace this with:

if (Commented)
  CommentOS << "\n";

or pull it into the previous if:

if (Commented) {
  if (AP->EnablePrintSchedInfo) {
    // If any comment was added above and we need sched info comment then
    // add this new comment just after the above comment w/o "\n" between them.
    CommentOS << " " << MF->getSubtarget().getSchedInfoStr(MI) << "\n";
    return true;
  }
  CommentOS << "\n";
}

[avt77]

The latest Simon's comments were implemented.

[RKSimon]

LGTM with one minor

lib/Target/X86/X86MCInstLower.cpp

107

EnablePrintSchedInfo &&

include/llvm/CodeGen/MachineInstr.h

 public:
   using mmo_iterator = MachineMemOperand **;
 
   /// Flags to specify different kinds of comments to output in
   /// assembly code.  These flags carry semantic information not
   /// otherwise easily derivable from the IR text.
   ///
   enum CommentFlag {
-    ReloadReuse = 0x1 // higher bits are reserved for target dep comments.
+    ReloadReuse = 0x1, // higher bits are reserved for target dep comments.
+    NoSchedComment = 0x2,
+    TAsmComments = 0x4    // Target Asm comments should start from this value
   };
 
   enum MIFlag {
     NoFlags      = 0,
     FrameSetup   = 1 << 0,              // Instruction is used as a part of
                                         // function frame setup code.
     FrameDestroy = 1 << 1,              // Instruction is used as a part of
                                         // function frame destruction code.

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

void AsmPrinter::EmitFunctionEntryLabel() {
   if (CurrentFnSym->isDefined())
     report_fatal_error("'" + Twine(CurrentFnSym->getName()) +
                        "' label emitted multiple times to assembly file");
 
   return OutStreamer->EmitLabel(CurrentFnSym);
 }
 
 /// emitComments - Pretty-print comments for instructions.
-static void emitComments(const MachineInstr &MI, raw_ostream &CommentOS,
+/// It returns true iff the sched comment was emitted.
+///   Otherwise it returns false.
+static bool emitComments(const MachineInstr &MI, raw_ostream &CommentOS,
                          AsmPrinter *AP) {
   const MachineFunction *MF = MI.getMF();
   const TargetInstrInfo *TII = MF->getSubtarget().getInstrInfo();
 
   // Check for spills and reloads
   int FI;
 
   const MachineFrameInfo &MFI = MF->getFrameInfo();
static bool emitComments(const MachineInstr &MI, raw_ostream &CommentOS,
   }
 
   // Check for spill-induced copies
   if (MI.getAsmPrinterFlag(MachineInstr::ReloadReuse)) {
     Commented = true;
     CommentOS << " Reload Reuse";
   }
 
-  if (Commented && AP->EnablePrintSchedInfo)
+  if (Commented && AP->EnablePrintSchedInfo) {
     // If any comment was added above and we need sched info comment then
     // add this new comment just after the above comment w/o "\n" between them.
     CommentOS << " " << MF->getSubtarget().getSchedInfoStr(MI) << "\n";
-  else if (Commented)
+    return true;
+  } else if (Commented)
     CommentOS << "\n";
+  return false;
 }
 
 /// emitImplicitDef - This method emits the specified machine instruction
 /// that is an implicit def.
 void AsmPrinter::emitImplicitDef(const MachineInstr *MI) const {
   unsigned RegNo = MI->getOperand(0).getReg();
 
   SmallString<128> Str;
for (auto &MI : MBB) {
         for (const HandlerInfo &HI : Handlers) {
           NamedRegionTimer T(HI.TimerName, HI.TimerDescription,
                              HI.TimerGroupName, HI.TimerGroupDescription,
                              TimePassesIsEnabled);
           HI.Handler->beginInstruction(&MI);
         }
       }
 
-      if (isVerbose())
-        emitComments(MI, OutStreamer->GetCommentOS(), this);
+      if (isVerbose() && emitComments(MI, OutStreamer->GetCommentOS(), this)) {
+        MachineInstr *MIP = const_cast<MachineInstr *>(&MI);
+        MIP->setAsmPrinterFlag(MachineInstr::NoSchedComment);
+      }
 
       switch (MI.getOpcode()) {
       case TargetOpcode::CFI_INSTRUCTION:
         emitCFIInstruction(MI);
         break;
       case TargetOpcode::LOCAL_ESCAPE:
         emitFrameAlloc(MI);
         break;
       case TargetOpcode::EH_LABEL:

lib/Target/X86/InstPrinter/X86InstComments.h

 // This defines functionality used to emit comments about X86 instructions to
 // an output stream for -fverbose-asm.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_TARGET_X86_INSTPRINTER_X86INSTCOMMENTS_H
 #define LLVM_LIB_TARGET_X86_INSTPRINTER_X86INSTCOMMENTS_H
 
+#include "llvm/CodeGen/MachineInstr.h"
+
 namespace llvm {
 
   enum AsmComments {
-    AC_EVEX_2_VEX = 0x2 // For instr that was compressed from EVEX to VEX.
+    // For instr that was compressed from EVEX to VEX.
+    AC_EVEX_2_VEX = MachineInstr::TAsmComments
   };
 
   class MCInst;
   class raw_ostream;
   bool EmitAnyX86InstComments(const MCInst *MI, raw_ostream &OS,
                               const char *(*getRegName)(unsigned));
 }
 
 #endif

lib/Target/X86/MCTargetDesc/X86BaseInfo.h

namespace X86 {
 
   /// The constants to describe instr prefixes if there are
   enum IPREFIXES {
     IP_NO_PREFIX = 0,
     IP_HAS_OP_SIZE = 1,
     IP_HAS_AD_SIZE = 2,
     IP_HAS_REPEAT_NE = 4,
     IP_HAS_REPEAT = 8,
-    IP_HAS_LOCK = 16
+    IP_HAS_LOCK = 16,
+    NO_SCHED_INFO = 32 // Don't add sched comment to the current instr because
+                       // it was already added
   };
 } // end namespace X86;
 
 /// X86II - This namespace holds all of the target specific flags that
 /// instruction info tracks.
 ///
 namespace X86II {
   /// Target Operand Flag enum.

lib/Target/X86/X86MCInstLower.cpp

void X86AsmPrinter::StackMapShadowTracker::emitShadowPadding(
   if (InShadow && CurrentShadowSize < RequiredShadowSize) {
     InShadow = false;
     EmitNops(OutStreamer, RequiredShadowSize - CurrentShadowSize,
              MF->getSubtarget<X86Subtarget>().is64Bit(), STI);
   }
 }
 
 void X86AsmPrinter::EmitAndCountInstruction(MCInst &Inst) {
-  OutStreamer->EmitInstruction(Inst, getSubtargetInfo(), EnablePrintSchedInfo);
+  OutStreamer->EmitInstruction(Inst, getSubtargetInfo(),
+                               EnablePrintSchedInfo &
+                                   !(Inst.getFlags() & X86::NO_SCHED_INFO));
   SMShadowTracker.count(Inst, getSubtargetInfo(), CodeEmitter.get());
 }
 
 X86MCInstLower::X86MCInstLower(const MachineFunction &mf,
                                X86AsmPrinter &asmprinter)
     : Ctx(mf.getContext()), MF(mf), TM(mf.getTarget()), MAI(*TM.getMCAsmInfo()),
       AsmPrinter(asmprinter) {}
 
if (auto *C = getConstantFromPool(*MI, MI->getOperand(4))) {
       }
       CS << "]";
       OutStreamer->AddComment(CS.str(), !EnablePrintSchedInfo);
     }
   }
 
   MCInst TmpInst;
   MCInstLowering.Lower(MI, TmpInst);
+  if (MI->getAsmPrinterFlag(MachineInstr::NoSchedComment))
+    TmpInst.setFlags(TmpInst.getFlags() | X86::NO_SCHED_INFO);
 
   // Stackmap shadows cannot include branch targets, so we can count the bytes
   // in a call towards the shadow, but must ensure that the no thread returns
   // in to the stackmap shadow.  The only way to achieve this is if the call
   // is at the end of the shadow.
   if (MI->isCall()) {
     // Count then size of the call towards the shadow
     SMShadowTracker.count(TmpInst, getSubtargetInfo(), CodeEmitter.get());

test/CodeGen/X86/avx512-schedule.ll

 declare void @func_f32(float)
 define <16 x float> @broadcast_ss_spill(float %x) {
 ; GENERIC-LABEL: broadcast_ss_spill:
 ; GENERIC:       # %bb.0:
 ; GENERIC-NEXT:    subq $24, %rsp # sched: [1:0.33]
 ; GENERIC-NEXT:    .cfi_def_cfa_offset 32
 ; GENERIC-NEXT:    vaddss %xmm0, %xmm0, %xmm0 # sched: [3:1.00]
 ; GENERIC-NEXT:    vmovaps %xmm0, (%rsp) # 16-byte Spill sched: [5:1.00]
-; GENERIC-NEXT:    # sched: [5:1.00]
 ; GENERIC-NEXT:    callq func_f32
 ; GENERIC-NEXT:    vbroadcastss (%rsp), %zmm0 # 16-byte Folded Reload sched: [5:1.00]
-; GENERIC-NEXT:    # sched: [5:1.00]
 ; GENERIC-NEXT:    addq $24, %rsp # sched: [1:0.33]
 ; GENERIC-NEXT:    retq # sched: [1:1.00]
 ;
 ; SKX-LABEL: broadcast_ss_spill:
 ; SKX:       # %bb.0:
 ; SKX-NEXT:    subq $24, %rsp # sched: [1:0.25]
 ; SKX-NEXT:    .cfi_def_cfa_offset 32
 ; SKX-NEXT:    vaddss %xmm0, %xmm0, %xmm0 # sched: [4:0.33]
 ; SKX-NEXT:    vmovaps %xmm0, (%rsp) # 16-byte Spill sched: [1:1.00]
-; SKX-NEXT:    # sched: [1:1.00]
 ; SKX-NEXT:    callq func_f32
 ; SKX-NEXT:    vbroadcastss (%rsp), %zmm0 # 16-byte Folded Reload sched: [8:0.50]
-; SKX-NEXT:    # sched: [8:0.50]
 ; SKX-NEXT:    addq $24, %rsp # sched: [1:0.25]
 ; SKX-NEXT:    retq # sched: [7:1.00]
   %a  = fadd float %x, %x
   call void @func_f32(float %a)
   %b = insertelement <16 x float> undef, float %a, i32 0
   %c = shufflevector <16 x float> %b, <16 x float> undef, <16 x i32> zeroinitializer
   ret <16 x float> %c
 }
 
 declare void @func_f64(double)
 define <8 x double> @broadcast_sd_spill(double %x) {
 ; GENERIC-LABEL: broadcast_sd_spill:
 ; GENERIC:       # %bb.0:
 ; GENERIC-NEXT:    subq $24, %rsp # sched: [1:0.33]
 ; GENERIC-NEXT:    .cfi_def_cfa_offset 32
 ; GENERIC-NEXT:    vaddsd %xmm0, %xmm0, %xmm0 # sched: [3:1.00]
 ; GENERIC-NEXT:    vmovapd %xmm0, (%rsp) # 16-byte Spill sched: [5:1.00]
-; GENERIC-NEXT:    # sched: [5:1.00]
 ; GENERIC-NEXT:    callq func_f64
 ; GENERIC-NEXT:    vbroadcastsd (%rsp), %zmm0 # 16-byte Folded Reload sched: [5:1.00]
-; GENERIC-NEXT:    # sched: [5:1.00]
 ; GENERIC-NEXT:    addq $24, %rsp # sched: [1:0.33]
 ; GENERIC-NEXT:    retq # sched: [1:1.00]
 ;
 ; SKX-LABEL: broadcast_sd_spill:
 ; SKX:       # %bb.0:
 ; SKX-NEXT:    subq $24, %rsp # sched: [1:0.25]
 ; SKX-NEXT:    .cfi_def_cfa_offset 32
 ; SKX-NEXT:    vaddsd %xmm0, %xmm0, %xmm0 # sched: [4:0.33]
 ; SKX-NEXT:    vmovapd %xmm0, (%rsp) # 16-byte Spill sched: [1:1.00]
-; SKX-NEXT:    # sched: [1:1.00]
 ; SKX-NEXT:    callq func_f64
 ; SKX-NEXT:    vbroadcastsd (%rsp), %zmm0 # 16-byte Folded Reload sched: [8:0.50]
-; SKX-NEXT:    # sched: [8:0.50]
 ; SKX-NEXT:    addq $24, %rsp # sched: [1:0.25]
 ; SKX-NEXT:    retq # sched: [7:1.00]
   %a  = fadd double %x, %x
   call void @func_f64(double %a)
   %b = insertelement <8 x double> undef, double %a, i32 0
   %c = shufflevector <8 x double> %b, <8 x double> undef, <8 x i32> zeroinitializer
   ret <8 x double> %c
 }