Fix for bug PR35549 - [X86] Repeated schedule comments

[avt77]

Summary

I fixed the issue.

[RKSimon]

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

745 ↗

(On Diff #125970)

update the comment to explain what the return value means

794 ↗

(On Diff #125970)

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

1045 ↗

(On Diff #125970)

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 ↗

(On Diff #125970)

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

lib/Target/X86/X86MCInstLower.cpp

107 ↗

(On Diff #125970)

Again, get rid of this hard coded constant

2007 ↗

(On Diff #125970)

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

[avt77]

lib/Target/X86/X86MCInstLower.cpp

2007 ↗

(On Diff #125970)

Yes, that's exactly what we need here.

[RKSimon]

lib/Target/X86/X86MCInstLower.cpp

2007 ↗

(On Diff #125970)

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

2007 ↗

(On Diff #125970)

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

796 ↗

(On Diff #126532)

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 ↗

(On Diff #126981)

EnablePrintSchedInfo &&

llvm/trunk/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;

llvm/trunk/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.

llvm/trunk/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());

llvm/trunk/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
 }