This is an archive of the discontinued LLVM Phabricator instance.

[X86] X86ATTInstPrinter: replace markup with startMarkup/endMarkup
AcceptedPublic

Authored by seiya on Jul 23 2019, 10:27 PM.

Event Timeline

seiya created this revision.Jul 23 2019, 10:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2019, 10:27 PM

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 marked an inline comment as done.Jul 25 2019, 1:36 AM

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 added inline comments.Jul 25 2019, 7:47 AM
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 marked 3 inline comments as done.Jul 31 2019, 2:50 AM
seiya added inline comments.
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.

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 updated this revision to Diff 212688.Jul 31 2019, 3:56 PM
seiya marked 3 inline comments as done.
  • Removed markup() from printRegName.
seiya added a comment.Jul 31 2019, 4:05 PM

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.

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

jhenderson accepted this revision.Aug 1 2019, 9:46 AM

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

This revision is now accepted and ready to land.Aug 1 2019, 9:46 AM
rupprecht accepted this revision.Aug 7 2019, 10:21 AM
rupprecht added inline comments.
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 planned changes to this revision.Aug 7 2019, 7:45 PM
seiya marked an inline comment as done.

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

seiya marked an inline comment as done.Aug 12 2019, 3:29 AM
seiya added inline comments.
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 added inline comments.Aug 12 2019, 8:47 AM
llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp
41–42

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

seiya updated this revision to Diff 215014.Aug 13 2019, 7:18 PM
seiya marked 2 inline comments as done.
  • Use withMarkup().
  • Remove printMarkedUpRegName (start/end/withMarkup are made const by changes in D65189).
This revision is now accepted and ready to land.Aug 13 2019, 7:18 PM
seiya marked an inline comment as done.Aug 13 2019, 7:21 PM
seiya added inline comments.
llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp
41–42

I made it const in D65189.

seiya updated this revision to Diff 215015.Aug 13 2019, 7:22 PM
  • Removed an unused declaration.
jhenderson added inline comments.Aug 14 2019, 8:05 AM
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 marked 3 inline comments as done.Aug 15 2019, 3:05 AM
seiya added inline comments.
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 accepted this revision.Aug 15 2019, 3:41 AM

LGTM.

llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp
379–381

Yes, that's what I meant.

rupprecht accepted this revision.Aug 15 2019, 10:53 AM
seiya marked 3 inline comments as done.Aug 16 2019, 2:28 AM
seiya added inline comments.
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 added inline comments.Aug 19 2019, 2:05 AM
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 updated this revision to Diff 216116.Aug 20 2019, 5:55 AM
seiya marked 3 inline comments as done.

Clang-formatted.

seiya marked an inline comment as done.Aug 20 2019, 5:56 AM
seiya added inline comments.
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 accepted this revision.Aug 20 2019, 6:15 AM

Okay, LGTM.