Fix for bug PR35549 - [X86] Repeated schedule comments

[avt77]

Summary

I fixed the issue.

[RKSimon]

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

745

update the comment to explain what the return value means

794

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

1045

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

2007

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

[avt77]

lib/Target/X86/X86MCInstLower.cpp

2007

Yes, that's exactly what we need here.

[RKSimon]

lib/Target/X86/X86MCInstLower.cpp

2007

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

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

797

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 &&

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,
+static bool emitComments(const MachineInstr &MI, raw_ostream &CommentOS,
                          AsmPrinter *AP) {
   const MachineFunction *MF = MI.getMF();
   const TargetInstrInfo *TII = MF->getSubtarget().getInstrInfo();
+  bool Result = false;
 
   // Check for spills and reloads
   int FI;
 
   const MachineFrameInfo &MFI = MF->getFrameInfo();
   bool Commented = false;
 
   // We assume a single instruction only has a spill or reload, not
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";
+    Result = true;
+  }
   else if (Commented)
     CommentOS << "\n";
+  return Result;
 }
 
 /// 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) {
           NamedRegionTimer T(HI.TimerName, HI.TimerDescription,
                              HI.TimerGroupName, HI.TimerGroupDescription,
                              TimePassesIsEnabled);
           HI.Handler->beginInstruction(&MI);
         }
       }
 
       if (isVerbose())
-        emitComments(MI, OutStreamer->GetCommentOS(), this);
+        if(emitComments(MI, OutStreamer->GetCommentOS(), this)) {
+          MachineInstr *MIP = const_cast<MachineInstr *>(&MI);
+          unsigned F = MI.getFlags() | 32; //X86::IP_NO_SCHED_INFO;
+          MIP->setFlags (F);
+        }
 
       switch (MI.getOpcode()) {
       case TargetOpcode::CFI_INSTRUCTION:
         emitCFIInstruction(MI);
         break;
       case TargetOpcode::LOCAL_ESCAPE:
         emitFrameAlloc(MI);
         break;

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,
+    IP_NO_SCHED_INFO = 32
   };
 } // 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() & 32));
   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);
+  TmpInst.setFlags(MI->getFlags());
 
   // 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
 }