Skip to content

[llvm] Adopt WithMarkup in the MIPS backend #65384

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 6, 2023
Merged

[llvm] Adopt WithMarkup in the MIPS backend #65384

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 9 additions & 10 deletions llvm/lib/Target/Mips/MCTargetDesc/MipsInstPrinter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@ const char* Mips::MipsFCCToString(Mips::CondCode CC) {
}

void MipsInstPrinter::printRegName(raw_ostream &OS, MCRegister Reg) const {
OS << markup("<reg:") << '$' << StringRef(getRegisterName(Reg)).lower()
<< markup(">");
markup(OS, Markup::Register)
<< '$' << StringRef(getRegisterName(Reg)).lower();
}

void MipsInstPrinter::printInst(const MCInst *MI, uint64_t Address,
Expand Down Expand Up @@ -134,7 +134,7 @@ void MipsInstPrinter::printOperand(const MCInst *MI, unsigned OpNo,
}

if (Op.isImm()) {
O << markup("<imm:") << formatImm(Op.getImm()) << markup(">");
markup(O, Markup::Immediate) << formatImm(Op.getImm());
return;
}

Expand All @@ -150,9 +150,9 @@ void MipsInstPrinter::printJumpOperand(const MCInst *MI, unsigned OpNo,
return printOperand(MI, OpNo, STI, O);

if (PrintBranchImmAsAddress)
O << markup("<imm:") << formatHex(Op.getImm()) << markup(">");
markup(O, Markup::Immediate) << formatHex(Op.getImm());
else
O << markup("<imm:") << formatImm(Op.getImm()) << markup(">");
markup(O, Markup::Immediate) << formatImm(Op.getImm());
}

void MipsInstPrinter::printBranchOperand(const MCInst *MI, uint64_t Address,
Expand All @@ -169,9 +169,9 @@ void MipsInstPrinter::printBranchOperand(const MCInst *MI, uint64_t Address,
Target &= 0xffffffff;
else if (STI.hasFeature(Mips::FeatureMips16))
Target &= 0xffff;
O << markup("<imm:") << formatHex(Target) << markup(">");
markup(O, Markup::Immediate) << formatHex(Target);
} else {
O << markup("<imm:") << formatImm(Op.getImm()) << markup(">");
markup(O, Markup::Immediate) << formatImm(Op.getImm());
}
}

Expand All @@ -184,7 +184,7 @@ void MipsInstPrinter::printUImm(const MCInst *MI, int opNum,
Imm -= Offset;
Imm &= (1 << Bits) - 1;
Imm += Offset;
O << markup("<imm:") << formatImm(Imm) << markup(">");
markup(O, Markup::Immediate) << formatImm(Imm);
return;
}

Expand Down Expand Up @@ -213,12 +213,11 @@ void MipsInstPrinter::printMemOperand(const MCInst *MI, int opNum,
break;
}

O << markup("<mem:");
WithMarkup M = markup(O, Markup::Memory);
Copy link
Contributor

@s-barannikov s-barannikov Sep 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is useless here because printOperand will change the color yet again and reset it to the default before returning.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It keeps the original behavior, but if you're saying that's wrong then I'm happy to update it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@JDevlieghere JDevlieghere Sep 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

printOperand(MI, opNum + 1, STI, O);
O << "(";
printOperand(MI, opNum, STI, O);
O << ")";
O << markup(">");
}

void MipsInstPrinter::printMemOperandEA(const MCInst *MI, int opNum,
Expand Down