[X86] X86ATTInstPrinter: replace markup with startMarkup/endMarkup

[jhenderson]

Is markup() still needed after this change?

llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp

38

This still seems to be using the old markup function?

41–42

Could printRegName just be made non-const?

[seiya]

In D65190#1599038, @jhenderson wrote:

Is markup() still needed after this change?

Yes we cannot remove markup() because it is used in other printers. ARMInstPrinter for example.

llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp

41–42

We can't make it non-const because printRegName is defined as a virtual const function in MCInstPrinter. How about marking MarkupState mutable instead?

[jhenderson]

llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp

41–42

Obvious follow-up question: can printRegName in the base class be made non-const, or does that cause too many issues further along?

If not, I think I'd prefer this version be kept.

[seiya]

llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp

41–42

I should have explain that as well. I've tried making it non-const but changes spread out (mainly around AsmParser) in the code base. printMarkedUpRegName is a simple solution for this.

[jhenderson]

Okay, the code change looks good to me. How do you test it?

I'd also like somebody else to give their opinion on the change.

[seiya]

• Removed markup() from printRegName.

[seiya]

In D65190#1608417, @jhenderson wrote:

Okay, the code change looks good to me. How do you test it?

llvm/test/MC/Disassembler/X86/marked-up.txt covers this part.

I'd also like somebody else to give their opinion on the change.

I'll add some reviewers based on the commit log.

[seiya]

Added some reviewers who seem to be familiar with this part.

[jhenderson]

Okay, LGTM, but please wait for someone else to confirm.

[rupprecht]

llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp

41–42

What if instead of adding this method, you make the MarkupSpans fields mutable so startMarkup/endMarkup can be marked const? This would enable markup support more broadly for other tools that call this method.
Alternatively, my other suggestion of replacing endMarkup() w/ a destructor on MarkupStart would allow MarkupStart itself to own the MarkupSpans, making it naturally const.

(I think this is fine to commit as-is and address in a later patch)

[seiya]

I'll try out the destructor approach suggested by @rupprecht in D65189.

[seiya]

llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp

41–42

What if instead of adding this method, you make the MarkupSpans fields mutable so startMarkup/endMarkup can be marked const?

I've suggested marking it mutable in an earlier comment, but @jhenderson prefers keeping printMarkedUpRegName instead. printRegName is the only method which needs to be const so this is not so bad change I think. I want to remove it if possible, btw.

Alternatively, my other suggestion of replacing endMarkup() w/ a destructor on MarkupStart would allow MarkupStart itself to own the MarkupSpans, making it naturally const.

I suppose that it can't be const: after all, it appends a new MarkupSpan into the output vector (which set by setMarkupSpans in D65189).

[jhenderson]

llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp

41–42

FWIW, I don't feel strongly about the use of mutable.

[seiya]

• Use withMarkup().
• Remove printMarkedUpRegName (start/end/withMarkup are made const by changes in D65189).

[seiya]

llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp

41–42

I made it const in D65189.

[seiya]

• Removed an unused declaration.

[jhenderson]

llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp

379–381

No need for this to be part of this change, but I find it slightly odd that you can't use the withMarkup pattern here. I take it that making the print function return a stream wouldn't be straightforward?

415–416

Could you do the following?

O << ',' << withMarkup(O, MarkupType::Imm) << ScaleVal;

[seiya]

llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp

379–381

I take it that making the print function return a stream wouldn't be straightforward?

Does "return a stream" mean raw_ostream &MCExpr::print(raw_ostream &OS, ...); instead of void MCExpr::print(...)?

415–416

No we can't for now. llvm::operator<<(raw_ostream &OS, const WithMarkup &M) is not implemented. I thought it could be confusing to use WithMarkup in such a way because it writes the start tag into the stream in its constructor, not in operator<<.

[jhenderson]

LGTM.

llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp

379–381

Yes, that's what I meant.

[seiya]

llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp

379–381

How can I utilize that returned stream? Op.getExpr()->print(O, &MAI) << withMarkup(MarkupType::Imm) <<'$'; came to my mind but it doesn't work because the Expr::print is evaluated before prepending the starting tag (<imm:) by withMarkup.

[jhenderson]

llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp

379–381

I might be misunderstanding something, but couldn't you do the following?

withMarkup(O, MarkupType::Imm) << '$' << Op.getExpr()->print(O, &MAI)

[seiya]

Clang-formatted.

[seiya]

llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp

379–381

No we can't. Op.getExpr()->print() prints the disassembly into O by itself and returns void. Making it return something streamable won't be straightforward to implement.

[jhenderson]

Okay, LGTM.

llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp

 
 #define DEBUG_TYPE "asm-printer"
 
 // Include the auto-generated portion of the assembly writer.
 #define PRINT_ALIAS_INSTR
 #include "X86GenAsmWriter.inc"
 
 void X86ATTInstPrinter::printRegName(raw_ostream &OS, unsigned RegNo) const {
-  OS << markup("<reg:") << '%' << getRegisterName(RegNo) << markup(">");
+  withMarkup(OS, MarkupType::Reg) << '%' << getRegisterName(RegNo);
 }
 
 void X86ATTInstPrinter::printInst(const MCInst *MI, raw_ostream &OS,
                                   StringRef Annot, const MCSubtargetInfo &STI) {
   // If verbose assembly is enabled, we can print some informative comments.
   if (CommentStream)
     HasCustomInstComment = EmitAnyX86InstComments(MI, *CommentStream, MII);
 
   printInstFlags(MI, OS);
 
   // Output CALLpcrel32 as "callq" in 64-bit mode.
   // In Intel annotation it's always emitted as "call".

 void X86ATTInstPrinter::printOperand(const MCInst *MI, unsigned OpNo,
                                      raw_ostream &O) {
   const MCOperand &Op = MI->getOperand(OpNo);
   if (Op.isReg()) {
     printRegName(O, Op.getReg());
   } else if (Op.isImm()) {
     // Print immediates as signed values.
     int64_t Imm = Op.getImm();
-    O << markup("<imm:") << '$' << formatImm(Imm) << markup(">");
+    withMarkup(O, MarkupType::Imm) << '$' << formatImm(Imm);
 
     // TODO: This should be in a helper function in the base class, so it can
     // be used by other printers.
 
     // If there are no instruction-specific comments, add a comment clarifying
     // the hex value of the immediate operand when it isn't in the range
     // [-256,255].
     if (CommentStream && !HasCustomInstComment && (Imm > 255 || Imm < -256)) {
       // Don't print unnecessary hex sign bits.
       if (Imm == (int16_t)(Imm))
         *CommentStream << format("imm = 0x%" PRIX16 "\n", (uint16_t)Imm);
       else if (Imm == (int32_t)(Imm))
         *CommentStream << format("imm = 0x%" PRIX32 "\n", (uint32_t)Imm);
       else
         *CommentStream << format("imm = 0x%" PRIX64 "\n", (uint64_t)Imm);
     }
   } else {
     assert(Op.isExpr() && "unknown operand kind in printOperand");
-    O << markup("<imm:") << '$';
+    O << startMarkup(MarkupType::Imm) << '$';
     Op.getExpr()->print(O, &MAI);
-    O << markup(">");
+    O << endMarkup();
   }
 }
 
 void X86ATTInstPrinter::printMemReference(const MCInst *MI, unsigned Op,
                                           raw_ostream &O) {
   const MCOperand &BaseReg = MI->getOperand(Op + X86::AddrBaseReg);
   const MCOperand &IndexReg = MI->getOperand(Op + X86::AddrIndexReg);
   const MCOperand &DispSpec = MI->getOperand(Op + X86::AddrDisp);
 
-  O << markup("<mem:");
+  O << startMarkup(MarkupType::Mem);
 
   // If this has a segment register, print it.
   printOptionalSegReg(MI, Op + X86::AddrSegmentReg, O);
 
   if (DispSpec.isImm()) {
     int64_t DispVal = DispSpec.getImm();
     if (DispVal || (!IndexReg.getReg() && !BaseReg.getReg()))
       O << formatImm(DispVal);
   } else {
     assert(DispSpec.isExpr() && "non-immediate displacement for LEA?");
     DispSpec.getExpr()->print(O, &MAI);
   }
 
   if (IndexReg.getReg() || BaseReg.getReg()) {
     O << '(';
     if (BaseReg.getReg())
       printOperand(MI, Op + X86::AddrBaseReg, O);
 
     if (IndexReg.getReg()) {
       O << ',';
       printOperand(MI, Op + X86::AddrIndexReg, O);
       unsigned ScaleVal = MI->getOperand(Op + X86::AddrScaleAmt).getImm();
       if (ScaleVal != 1) {
-        O << ',' << markup("<imm:") << ScaleVal // never printed in hex.
-          << markup(">");
+        O << ',';
+        withMarkup(O, MarkupType::Imm) << ScaleVal; // never printed in hex.
       }
     }
     O << ')';
   }
 
-  O << markup(">");
+  O << endMarkup();
 }
 
 void X86ATTInstPrinter::printSrcIdx(const MCInst *MI, unsigned Op,
                                     raw_ostream &O) {
-  O << markup("<mem:");
+  O << startMarkup(MarkupType::Mem);
 
   // If this has a segment register, print it.
   printOptionalSegReg(MI, Op + 1, O);
 
   O << "(";
   printOperand(MI, Op, O);
   O << ")";
 
-  O << markup(">");
+  O << endMarkup();
 }
 
 void X86ATTInstPrinter::printDstIdx(const MCInst *MI, unsigned Op,
                                     raw_ostream &O) {
-  O << markup("<mem:");
+  O << startMarkup(MarkupType::Mem);
 
   O << "%es:(";
   printOperand(MI, Op, O);
   O << ")";
 
-  O << markup(">");
+  O << endMarkup();
 }
 
 void X86ATTInstPrinter::printMemOffset(const MCInst *MI, unsigned Op,
                                        raw_ostream &O) {
   const MCOperand &DispSpec = MI->getOperand(Op);
 
-  O << markup("<mem:");
+  O << startMarkup(MarkupType::Mem);
 
   // If this has a segment register, print it.
   printOptionalSegReg(MI, Op + 1, O);
 
   if (DispSpec.isImm()) {
     O << formatImm(DispSpec.getImm());
   } else {
     assert(DispSpec.isExpr() && "non-immediate displacement?");
     DispSpec.getExpr()->print(O, &MAI);
   }
 
-  O << markup(">");
+  O << endMarkup();
 }
 
 void X86ATTInstPrinter::printU8Imm(const MCInst *MI, unsigned Op,
                                    raw_ostream &O) {
   if (MI->getOperand(Op).isExpr())
     return printOperand(MI, Op, O);
 
-  O << markup("<imm:") << '$' << formatImm(MI->getOperand(Op).getImm() & 0xff)
-    << markup(">");
+  withMarkup(O, MarkupType::Imm) << '$'
+    << formatImm(MI->getOperand(Op).getImm() & 0xff);
 }
 
 void X86ATTInstPrinter::printSTiRegOperand(const MCInst *MI, unsigned OpNo,
                                            raw_ostream &OS) {
   const MCOperand &Op = MI->getOperand(OpNo);
   unsigned Reg = Op.getReg();
   // Override the default printing to print st(0) instead st.
   if (Reg == X86::ST0)
-    OS << markup("<reg:") << "%st(0)" << markup(">");
+    withMarkup(OS, MarkupType::Reg) << "%st(0)";
   else
     printRegName(OS, Reg);
 }