[llvm] Adopt WithMarkup in the MIPS backend #65384
Conversation
Adopt the new markup overload, introduced in 77d1032, in the MIPS backend.
JDevlieghere
requested review from
dsandersllvm,
antoniofrighetto and
s-barannikov
| @@ -213,12 +213,11 @@ void MipsInstPrinter::printMemOperand(const MCInst *MI, int opNum, | |||
| break; | |||
| } | |||
|
|
|||
| O << markup("<mem:"); | |||
| WithMarkup M = markup(O, Markup::Memory); | |||
There was a problem hiding this comment.
This is useless here because printOperand will change the color yet again and reset it to the default before returning.
There was a problem hiding this comment.
It keeps the original behavior, but if you're saying that's wrong then I'm happy to update it.
There was a problem hiding this comment.
Ah, I was thinking about colors and forgot about the "<mem:" part.
I don't know what the markup has beed used for and whether nested tags are allowed. I was trying to say that the memory operand will not have Memory style. Instead, it will be displayed as Immediate + Register or Register + Register (and default-style parentheses). If that's OK from lldb's point of view, I don't see other issues.
There was a problem hiding this comment.
LLDB uses the ANSI escape characters for coloring, not the markup annotations so for LLDB this becomes a NOOP as you pointed out. I don't know who relies on the markup annotations but it's probably best to keep the existing behavior to avoid breaking any tools that might be relying on it.
There was a problem hiding this comment.
Looks fine to me as long as the memory operands are printed the way lldb wants, though I have no relation to the MIPS backend.
Here is the proposal for annotated assembly output and the initial implementation.
PS I wouldn't mind if the support for marked-up disassembly was removed in favor of colored up disassembly. I doubt that there are any tools that actually parse the markup.
| @@ -213,12 +213,11 @@ void MipsInstPrinter::printMemOperand(const MCInst *MI, int opNum, | |||
| break; | |||
| } | |||
|
|
|||
| O << markup("<mem:"); | |||
| WithMarkup M = markup(O, Markup::Memory); | |||
There was a problem hiding this comment.
I'm not familiar with WithMarkup but I don't think it's very clear that this (I assume) affects the stream until it's destroyed
There was a problem hiding this comment.
Yeah, something like make_scoped_markup would've been clearer at the cost of being more verbose. Given that most uses use operator << I decided to prioritize readability for those cases.
Local branch amd-gfx 1b3f7c5 Merged main:ddbcc10b9e26 into amd-gfx:632a231f2ac6 Remote branch main c473215 [llvm] Adopt WithMarkup in the MIPS backend (llvm#65384)
Adopt the new markup overload, introduced in 77d1032, in the MIPS backend.
Adopt the new markup overload, introduced in 77d1032, in the MIPS backend.