Fix for bug PR35549 - [X86] Repeated schedule comments

[avt77]

Summary

I fixed the issue.

[RKSimon]

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

725–727

update the comment to explain what the return value means

771

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

1026

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

104

Again, get rid of this hard coded constant

2005

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

[avt77]

lib/Target/X86/X86MCInstLower.cpp

2005

Yes, that's exactly what we need here.

[RKSimon]

lib/Target/X86/X86MCInstLower.cpp

2005

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

2005

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 ↗

(On Diff #126532)

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

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

772

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

104

EnablePrintSchedInfo &&

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 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.
+  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";
-  else if (Commented)
+      return true;
+    }
     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;
   raw_svector_ostream OS(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/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
 }