Details
- Reviewers
jhenderson rupprecht craig.topper enderby
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 35554 Build 35553: arc lint + arc unit
Event Timeline
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? |
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. |
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.
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.
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. (I think this is fine to commit as-is and address in a later patch) |
llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp | ||
---|---|---|
41–42 |
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.
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). |
llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp | ||
---|---|---|
41–42 | FWIW, I don't feel strongly about the use of mutable. |
- Use withMarkup().
- Remove printMarkedUpRegName (start/end/withMarkup are made const by changes in D65189).
llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp | ||
---|---|---|
388–390 | 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? | |
424–427 | Could you do the following? O << ',' << withMarkup(O, MarkupType::Imm) << ScaleVal; |
llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp | ||
---|---|---|
388–390 |
Does "return a stream" mean raw_ostream &MCExpr::print(raw_ostream &OS, ...); instead of void MCExpr::print(...)? | |
424–427 | 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<<. |
LGTM.
llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp | ||
---|---|---|
388–390 | Yes, that's what I meant. |
llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp | ||
---|---|---|
388–390 | 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. |
llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp | ||
---|---|---|
388–390 | I might be misunderstanding something, but couldn't you do the following? withMarkup(O, MarkupType::Imm) << '$' << Op.getExpr()->print(O, &MAI) |
llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp | ||
---|---|---|
388–390 | 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. |
This still seems to be using the old markup function?