[SystemZInstPrinter] Introduce markup tags emission

[antoniofrighetto]

Summary

SystemZ assembly syntax emission now leverages markup tags, if enabled.

[antoniofrighetto]

Test added.

[uweigand]

Can you explain what this is, and where this marked-up assembler could be useful? I see so far only ARM supports that feature, but I don't really understand the point ...

[antoniofrighetto]

@uweigand, mostly third-party clients such as disassemblers and pretty printers, as per official documentation. Already supported by ARM as well as x64.

[antoniofrighetto]

Fixed a small regression.

[uweigand]

In D129868#3751791, @antoniofrighetto wrote:

@uweigand, mostly third-party clients such as disassemblers and pretty printers, as per official documentation. Already supported by ARM as well as x64.

Do you have any example of such tools? The reason I'm asking is that it's a bit hard for me to judge whether the implementation is fully complete and would make those uses cases actually work, without seeing any.

As a comment on the implementation: I think it would be much preferable to use the base class UseMarkup member (or even better, the markup helper), instead of having to explicitly pass on the extra argument. I guess the reason you're not doing this is because of the calls from PrintAsmOperand / PrintMemAsmOperand? This actually makes me wonder if it wouldn't be cleaner to just duplicate the (small amount of) code in printOperand and printAddress into PrintAsmOperand and PrintMemAsmOperand, and then stop making all those SystemZInstrPrinter routines static ...

[antoniofrighetto]

@uweigand, we leverage LLVM disassembler (and, specifically, MCInstPrinter::printInst) as part of our static binary translator tool, here, though I assume I could count many other uses that I'm not aware of (we've updated other targets too in order to support markup).

I'm actually using UseMarkup member field anywhere, except for the static functions – whose callers are PrintAsmOperand / PrintMemAsmOperand, as you noted – for which, clearly, I could not. I reckon it's not really that big of a deal, being limited to a very small portion of the code.

Test fixed.

[antoniofrighetto]

@MaskRay, test added for this patch too, failure of unit tests seems completely unrelated to the change.

[antoniofrighetto]

@MaskRay, as there do not seem to be further concerns, could you kindly commit this on my behalf please (I do not have commit access)?

[MaskRay]

In D129868#3800796, @antoniofrighetto wrote:

@MaskRay, as there do not seem to be further concerns, could you kindly commit this on my behalf please (I do not have commit access)?

I've LGTM but we need to wait a bit for @uweigand

[uweigand]

Sorry for the delay, I was traveling last week. Thanks for the use case, it's good to have some validation that the output in this form is actually usable.

As to the implementation, I still believe it would be better to eliminate the static routines as outlined in my last comment. Do you think you could give that a try?

[antoniofrighetto]

Hello @uweigand, I apologize for being late on this. From the build, it looks like some systemz tests are failing because the register name is actually not emitted. Is it possible that I'm not handling properly it via SystemZInstPrinter::getRegisterName? Do you have any clue by any chance? The duplicated code in SystemZAsmPrinter.cpp looks correct to me.

[uweigand]

llvm/lib/Target/SystemZ/MCTargetDesc/SystemZInstPrinter.cpp

77–80

You've removed the RegName here.

91–92

Could you make those non-static as well, so we don't have to pass in UseMarkup explicitly.

llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp

796

And RegName is also missing here.

[antoniofrighetto]

llvm/lib/Target/SystemZ/MCTargetDesc/SystemZInstPrinter.cpp

77–80

Argh, how could I not even notice? Fixed, thanks for noticing it.

91–92

Sure!

[uweigand]

LGTM now! Thanks for making those changes.

[antoniofrighetto]

@uweigand, thanks for reviewing them! Might I ask you to commit it on my behalf, please (as I do not have commit access, account associated to GitHub)? Thank you.

llvm/lib/Target/SystemZ/MCTargetDesc/SystemZInstPrinter.h

public:
   // Automatically generated by tblgen.
   std::pair<const char *, uint64_t> getMnemonic(const MCInst *MI) override;
   void printInstruction(const MCInst *MI, uint64_t Address, raw_ostream &O);
   static const char *getRegisterName(unsigned RegNo);
 
   // Print an address with the given base, displacement and index.
   static void printAddress(const MCAsmInfo *MAI, unsigned Base,
                            const MCOperand &DispMO, unsigned Index,
-                           raw_ostream &O);
+                           raw_ostream &O, bool UseMarkup);
 
   // Print the given operand.
   static void printOperand(const MCOperand &MO, const MCAsmInfo *MAI,
-                           raw_ostream &O);
+                           raw_ostream &O, bool UseMarkup);
 
   static void printFormattedRegName(const MCAsmInfo *MAI, unsigned RegNo,
-                                    raw_ostream &O);
+                                    raw_ostream &O, bool UseMarkup);
 
   // Override MCInstPrinter.
   inline void printRegName(raw_ostream &O, unsigned RegNo) const override {
-    printFormattedRegName(&MAI, RegNo, O);
+    printFormattedRegName(&MAI, RegNo, O, true);
   }
 
   void printInst(const MCInst *MI, uint64_t Address, StringRef Annot,
                  const MCSubtargetInfo &STI, raw_ostream &O) override;
 
 private:
   // Print various types of operand.
   void printOperand(const MCInst *MI, int OpNum, raw_ostream &O);

llvm/lib/Target/SystemZ/MCTargetDesc/SystemZInstPrinter.cpp

 using namespace llvm;
 
 #define DEBUG_TYPE "asm-printer"
 
 #include "SystemZGenAsmWriter.inc"
 
 void SystemZInstPrinter::printAddress(const MCAsmInfo *MAI, unsigned Base,
                                       const MCOperand &DispMO, unsigned Index,
-                                      raw_ostream &O) {
-  printOperand(DispMO, MAI, O);
+                                      raw_ostream &O, bool UseMarkup) {
+  printOperand(DispMO, MAI, O, UseMarkup);
   if (Base || Index) {
     O << '(';
     if (Index) {
-      printFormattedRegName(MAI, Index, O);
+      printFormattedRegName(MAI, Index, O, UseMarkup);
       if (Base)
         O << ',';
     }
     if (Base)
-      printFormattedRegName(MAI, Base, O);
+      printFormattedRegName(MAI, Base, O, UseMarkup);
     O << ')';
   }
 }
 
 void SystemZInstPrinter::printOperand(const MCOperand &MO, const MCAsmInfo *MAI,
-                                      raw_ostream &O) {
+                                      raw_ostream &O, bool UseMarkup) {
   if (MO.isReg()) {
     if (!MO.getReg())
       O << '0';
     else
-      printFormattedRegName(MAI, MO.getReg(), O);
-  }
-  else if (MO.isImm())
+      printFormattedRegName(MAI, MO.getReg(), O, UseMarkup);
+  } else if (MO.isImm()) {
+    if (UseMarkup)
+      O << "<imm:";
     O << MO.getImm();
-  else if (MO.isExpr())
+    if (UseMarkup)
+      O << ">";
+  } else if (MO.isExpr())
     MO.getExpr()->print(O, MAI);
   else
     llvm_unreachable("Invalid operand");
 }
 
 void SystemZInstPrinter::printFormattedRegName(const MCAsmInfo *MAI,
-                                               unsigned RegNo, raw_ostream &O) {
+                                               unsigned RegNo, raw_ostream &O,
+                                               bool UseMarkup) {
   const char *RegName = getRegisterName(RegNo);
   if (MAI->getAssemblerDialect() == AD_HLASM) {
     // Skip register prefix so that only register number is left
     assert(isalpha(RegName[0]) && isdigit(RegName[1]));
+    if (UseMarkup)
+      O << "<reg:";
     O << (RegName + 1);
-  } else
+    if (UseMarkup)
+      O << ">";
+  } else {
+    if (UseMarkup)
+      O << "<reg:";
     O << '%' << RegName;
+    if (UseMarkup)
+      O << ">";
+  }
 }
 
 void SystemZInstPrinter::printInst(const MCInst *MI, uint64_t Address,
                                    StringRef Annot, const MCSubtargetInfo &STI,
                                    raw_ostream &O) {
   printInstruction(MI, Address, O);
   printAnnotation(O, Annot);
 }
 
 template <unsigned N>
-static void printUImmOperand(const MCInst *MI, int OpNum, raw_ostream &O) {
+static void printUImmOperand(const MCInst *MI, int OpNum, raw_ostream &O,
+                             bool UseMarkup) {
   int64_t Value = MI->getOperand(OpNum).getImm();
   assert(isUInt<N>(Value) && "Invalid uimm argument");
+  if (UseMarkup)
+    O << "<imm:";
   O << Value;
+  if (UseMarkup)
+    O << ">";
 }
 
 template <unsigned N>
-static void printSImmOperand(const MCInst *MI, int OpNum, raw_ostream &O) {
+static void printSImmOperand(const MCInst *MI, int OpNum, raw_ostream &O,
+                             bool UseMarkup) {
   int64_t Value = MI->getOperand(OpNum).getImm();
   assert(isInt<N>(Value) && "Invalid simm argument");
+  if (UseMarkup)
+    O << "<imm:";
   O << Value;
+  if (UseMarkup)
+    O << ">";
 }
 
 void SystemZInstPrinter::printU1ImmOperand(const MCInst *MI, int OpNum,
                                            raw_ostream &O) {
-  printUImmOperand<1>(MI, OpNum, O);
+  printUImmOperand<1>(MI, OpNum, O, UseMarkup);
 }
 
 void SystemZInstPrinter::printU2ImmOperand(const MCInst *MI, int OpNum,
                                            raw_ostream &O) {
-  printUImmOperand<2>(MI, OpNum, O);
+  printUImmOperand<2>(MI, OpNum, O, UseMarkup);
 }
 
 void SystemZInstPrinter::printU3ImmOperand(const MCInst *MI, int OpNum,
                                            raw_ostream &O) {
-  printUImmOperand<3>(MI, OpNum, O);
+  printUImmOperand<3>(MI, OpNum, O, UseMarkup);
 }
 
 void SystemZInstPrinter::printU4ImmOperand(const MCInst *MI, int OpNum,
                                            raw_ostream &O) {
-  printUImmOperand<4>(MI, OpNum, O);
+  printUImmOperand<4>(MI, OpNum, O, UseMarkup);
 }
 
 void SystemZInstPrinter::printU6ImmOperand(const MCInst *MI, int OpNum,
                                            raw_ostream &O) {
-  printUImmOperand<6>(MI, OpNum, O);
+  printUImmOperand<6>(MI, OpNum, O, UseMarkup);
 }
 
 void SystemZInstPrinter::printS8ImmOperand(const MCInst *MI, int OpNum,
                                            raw_ostream &O) {
-  printSImmOperand<8>(MI, OpNum, O);
+  printSImmOperand<8>(MI, OpNum, O, UseMarkup);
 }
 
 void SystemZInstPrinter::printU8ImmOperand(const MCInst *MI, int OpNum,
                                            raw_ostream &O) {
-  printUImmOperand<8>(MI, OpNum, O);
+  printUImmOperand<8>(MI, OpNum, O, UseMarkup);
 }
 
 void SystemZInstPrinter::printU12ImmOperand(const MCInst *MI, int OpNum,
                                             raw_ostream &O) {
-  printUImmOperand<12>(MI, OpNum, O);
+  printUImmOperand<12>(MI, OpNum, O, UseMarkup);
 }
 
 void SystemZInstPrinter::printS16ImmOperand(const MCInst *MI, int OpNum,
                                             raw_ostream &O) {
-  printSImmOperand<16>(MI, OpNum, O);
+  printSImmOperand<16>(MI, OpNum, O, UseMarkup);
 }
 
 void SystemZInstPrinter::printU16ImmOperand(const MCInst *MI, int OpNum,
                                             raw_ostream &O) {
-  printUImmOperand<16>(MI, OpNum, O);
+  printUImmOperand<16>(MI, OpNum, O, UseMarkup);
 }
 
 void SystemZInstPrinter::printS32ImmOperand(const MCInst *MI, int OpNum,
                                             raw_ostream &O) {
-  printSImmOperand<32>(MI, OpNum, O);
+  printSImmOperand<32>(MI, OpNum, O, UseMarkup);
 }
 
 void SystemZInstPrinter::printU32ImmOperand(const MCInst *MI, int OpNum,
                                             raw_ostream &O) {
-  printUImmOperand<32>(MI, OpNum, O);
+  printUImmOperand<32>(MI, OpNum, O, UseMarkup);
 }
 
 void SystemZInstPrinter::printU48ImmOperand(const MCInst *MI, int OpNum,
                                             raw_ostream &O) {
-  printUImmOperand<48>(MI, OpNum, O);
+  printUImmOperand<48>(MI, OpNum, O, UseMarkup);
 }
 
 void SystemZInstPrinter::printPCRelOperand(const MCInst *MI, int OpNum,
                                            raw_ostream &O) {
   const MCOperand &MO = MI->getOperand(OpNum);
   if (MO.isImm()) {
-    O << "0x";
+    O << markup("<imm:") << "0x";
     O.write_hex(MO.getImm());
+    O << markup(">");
   } else
     MO.getExpr()->print(O, &MAI);
 }
 
 void SystemZInstPrinter::printPCRelTLSOperand(const MCInst *MI,
                                               uint64_t Address, int OpNum,
                                               raw_ostream &O) {
   // Output the PC-relative operand.
switch (refExp.getKind()) {
         llvm_unreachable("Unexpected symbol kind");
     }
     O << refExp.getSymbol().getName();
   }
 }
 
 void SystemZInstPrinter::printOperand(const MCInst *MI, int OpNum,
                                       raw_ostream &O) {
-  printOperand(MI->getOperand(OpNum), &MAI, O);
+  printOperand(MI->getOperand(OpNum), &MAI, O, UseMarkup);
 }
 
 void SystemZInstPrinter::printBDAddrOperand(const MCInst *MI, int OpNum,
                                             raw_ostream &O) {
   printAddress(&MAI, MI->getOperand(OpNum).getReg(), MI->getOperand(OpNum + 1),
-               0, O);
+               0, O, UseMarkup);
 }
 
 void SystemZInstPrinter::printBDXAddrOperand(const MCInst *MI, int OpNum,
                                              raw_ostream &O) {
   printAddress(&MAI, MI->getOperand(OpNum).getReg(), MI->getOperand(OpNum + 1),
-               MI->getOperand(OpNum + 2).getReg(), O);
+               MI->getOperand(OpNum + 2).getReg(), O, UseMarkup);
 }
 
 void SystemZInstPrinter::printBDLAddrOperand(const MCInst *MI, int OpNum,
                                              raw_ostream &O) {
   unsigned Base = MI->getOperand(OpNum).getReg();
   const MCOperand &DispMO = MI->getOperand(OpNum + 1);
   uint64_t Length = MI->getOperand(OpNum + 2).getImm();
-  printOperand(DispMO, &MAI, O);
+  printOperand(DispMO, &MAI, O, UseMarkup);
   O << '(' << Length;
   if (Base) {
     O << ",";
     printRegName(O, Base);
   }
   O << ')';
 }
 
 void SystemZInstPrinter::printBDRAddrOperand(const MCInst *MI, int OpNum,
                                              raw_ostream &O) {
   unsigned Base = MI->getOperand(OpNum).getReg();
   const MCOperand &DispMO = MI->getOperand(OpNum + 1);
   unsigned Length = MI->getOperand(OpNum + 2).getReg();
-  printOperand(DispMO, &MAI, O);
+  printOperand(DispMO, &MAI, O, UseMarkup);
   O << "(";
   printRegName(O, Length);
   if (Base) {
     O << ",";
     printRegName(O, Base);
   }
   O << ')';
 }
 
 void SystemZInstPrinter::printBDVAddrOperand(const MCInst *MI, int OpNum,
                                              raw_ostream &O) {
   printAddress(&MAI, MI->getOperand(OpNum).getReg(), MI->getOperand(OpNum + 1),
-               MI->getOperand(OpNum + 2).getReg(), O);
+               MI->getOperand(OpNum + 2).getReg(), O, UseMarkup);
 }
 
 void SystemZInstPrinter::printCond4Operand(const MCInst *MI, int OpNum,
                                            raw_ostream &O) {
   static const char *const CondNames[] = {
     "o", "h", "nle", "l", "nhe", "lh", "ne",
     "e", "nlh", "he", "nl", "le", "nh", "no"
   };
   uint64_t Imm = MI->getOperand(OpNum).getImm();
   assert(Imm > 0 && Imm < 15 && "Invalid condition");
   O << CondNames[Imm - 1];
 }

llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp

 bool SystemZAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
                                         const char *ExtraCode,
                                         raw_ostream &OS) {
   const MCRegisterInfo &MRI = *TM.getMCRegisterInfo();
   const MachineOperand &MO = MI->getOperand(OpNo);
   MCOperand MCOp;
   if (ExtraCode) {
     if (ExtraCode[0] == 'N' && !ExtraCode[1] && MO.isReg() &&
         SystemZ::GR128BitRegClass.contains(MO.getReg()))
       MCOp =
           MCOperand::createReg(MRI.getSubReg(MO.getReg(), SystemZ::subreg_l64));
     else
       return AsmPrinter::PrintAsmOperand(MI, OpNo, ExtraCode, OS);
   } else {
     SystemZMCInstLower Lower(MF->getContext(), *this);
     MCOp = Lower.lowerOperand(MO);
   }
-  SystemZInstPrinter::printOperand(MCOp, MAI, OS);
+  SystemZInstPrinter::printOperand(MCOp, MAI, OS, false);
   return false;
 }
 
 bool SystemZAsmPrinter::PrintAsmMemoryOperand(const MachineInstr *MI,
                                               unsigned OpNo,
                                               const char *ExtraCode,
                                               raw_ostream &OS) {
-  SystemZInstPrinter::
-    printAddress(MAI, MI->getOperand(OpNo).getReg(),
-                 MCOperand::createImm(MI->getOperand(OpNo + 1).getImm()),
-                 MI->getOperand(OpNo + 2).getReg(), OS);
+  SystemZInstPrinter::printAddress(
+      MAI, MI->getOperand(OpNo).getReg(),
+      MI->getOperand(OpNo + 2).getReg(), OS, false);
   return false;
 }
 
 void SystemZAsmPrinter::emitEndOfAsmFile(Module &M) {
   emitStackMaps(SM);
 }
 
 void SystemZAsmPrinter::emitFunctionBodyEnd() {

llvm/test/MC/Disassembler/SystemZ/marked-up.txt

+# RUN: llvm-mc --mdis %s -triple=s390x-linux-gnu -mcpu=zEC12 2>&1 | FileCheck %s
+
+# CHECK: blr  <reg:%r10>
+0x07 0x4a
+# CHECK: lrl  <reg:%r15>, <imm:0x8e>
+0xc4 0xfd 0x00 0x00 0x00 0x47
+# CHECK: stc  <reg:%r0>, <imm:4095>
+0x42 0x00 0x0f 0xff
+# CHECK: trace  <reg:%r0>, <reg:%r0>, <imm:0>(<reg:%r1>)
+0x99 0x00 0x10 0x00
+