[PATCH] MIR Serialization: Serialize MBB Successors.
Duncan P. N. Exon Smith
dexonsmith at apple.com
Mon Jun 29 10:37:48 PDT 2015
> On 2015-Jun-29, at 10:20, Alex Lorenz <arphaman at gmail.com> wrote:
>
> I rebased this patch on top of trunk.
>
>
> REPOSITORY
> rL LLVM
>
> http://reviews.llvm.org/D10699
>
> Files:
> include/llvm/CodeGen/MIRYamlMapping.h
> lib/CodeGen/MIRParser/MIParser.cpp
> lib/CodeGen/MIRParser/MIParser.h
> lib/CodeGen/MIRParser/MIRParser.cpp
> lib/CodeGen/MIRPrinter.cpp
> test/CodeGen/MIR/expected-eof-after-successor-mbb.mir
> test/CodeGen/MIR/expected-mbb-reference-for-successor-mbb.mir
> test/CodeGen/MIR/successor-basic-blocks.mir
>
> EMAIL PREFERENCES
> http://reviews.llvm.org/settings/panel/emailpreferences/
> <D10699.28684.patch>
> Index: lib/CodeGen/MIRParser/MIParser.cpp
> ===================================================================
> --- lib/CodeGen/MIRParser/MIParser.cpp
> +++ lib/CodeGen/MIRParser/MIParser.cpp
> @@ -64,10 +64,12 @@
> bool error(StringRef::iterator Loc, const Twine &Msg);
>
> MachineInstr *parse();
> + MachineBasicBlock *parseMBB();
Strange to break the pattern here. Actually, strange that
`MachineInstr` doesn't return `bool`, too.
More below.
>
> bool parseRegister(unsigned &Reg);
> bool parseRegisterOperand(MachineOperand &Dest, bool IsDef = false);
> bool parseImmediateOperand(MachineOperand &Dest);
> + MachineBasicBlock *parseMBBReference();
Strange to break the pattern here as well.
> bool parseMBBOperand(MachineOperand &Dest);
> bool parseGlobalAddressOperand(MachineOperand &Dest);
> bool parseMachineOperand(MachineOperand &Dest);
> Index: lib/CodeGen/MIRParser/MIRParser.cpp
> ===================================================================
> --- lib/CodeGen/MIRParser/MIRParser.cpp
> +++ lib/CodeGen/MIRParser/MIRParser.cpp
> @@ -262,9 +262,21 @@
> if (YamlMBB.AddressTaken)
> MBB.setHasAddressTaken();
> MBB.setIsLandingPad(YamlMBB.IsLandingPad);
> + SMDiagnostic Error;
> + // Parse the successors.
> + for (const auto &MBBSource : YamlMBB.Successors) {
> + if (auto *SuccMBB = parseMBBReference(SM, MF, MBBSource.Value, MBBSlots,
> + IRSlots, Error)) {
> + // TODO: Report an error when the successor is self.
> + // TODO: Report an error when adding the same successor more than once.
> + MBB.addSuccessor(SuccMBB);
> + continue;
> + }
> + reportDiagnostic(diagFromMIStringDiag(Error, MBBSource.SourceRange));
> + return true;
IIRC, previously you've been using this pattern:
MachineBasicBlock *SuccMBB;
if (parseMBBReference(SuccMBB, SM, MF, MBBSource.Value, MBBSlots, IRSlots, Error))
return error(Error, MBBSource.SourceRange);
// TODO: Report an error when the successor is self.
// TODO: Report an error when adding the same successor more than once.
MBB.addSuccessor(SuccMBB);
This helps prevent nesting for the error-free control flow, which is a
real concern given that your TODOs list more errors you're going to have
to add. Besides, patterns are more valuable if you're consistent.
Is there a good reason to do it differently here?
If there isn't, I just noticed that `parse()` (which returns a
`MachineInstr*`) is already breaking the pattern. Unless you have a
good reason for that please change it to returning `bool` and taking a
`MachineInstr*&` as well.
Note: I've also packed the `reportDiagnostic()`,
`diagFromMIStringDig()`, and `return true` together into a function
called `error()`. Kind of a separate issue, but I think you should find
a way to do something like this. Seems like a lot of boilerplate.
> + }
> // Parse the instructions.
> for (const auto &MISource : YamlMBB.Instructions) {
> - SMDiagnostic Error;
> if (auto *MI = parseMachineInstr(SM, MF, MISource.Value, MBBSlots, IRSlots,
> Error)) {
> MBB.insert(MBB.end(), MI);
> Index: lib/CodeGen/MIRPrinter.cpp
> ===================================================================
> --- lib/CodeGen/MIRPrinter.cpp
> +++ lib/CodeGen/MIRPrinter.cpp
> @@ -193,6 +200,14 @@
> llvm_unreachable("Can't print this kind of register yet");
> }
>
> +void MIPrinter::printMBBReference(const MachineBasicBlock &MBB) {
> + OS << "%bb." << MBB.getNumber();
> + if (const auto *BB = MBB.getBasicBlock()) {
> + if (BB->hasName())
> + OS << '.' << BB->getName();
> + }
> +}
> +
> void MIPrinter::print(const MachineOperand &Op, const TargetRegisterInfo *TRI) {
> switch (Op.getType()) {
> case MachineOperand::MO_Register:
> @@ -204,11 +219,7 @@
> OS << Op.getImm();
> break;
> case MachineOperand::MO_MachineBasicBlock:
> - OS << "%bb." << Op.getMBB()->getNumber();
> - if (const auto *BB = Op.getMBB()->getBasicBlock()) {
> - if (BB->hasName())
> - OS << '.' << BB->getName();
> - }
> + printMBBReference(*Op.getMBB());
This part of the commit -- splitting out a `printMBBReference()`
function -- looks like NFC. Please commit this (and anything else
that's not changing behaviour) separately. Otherwise, it's hard to tell
what's a real change and what isn't.
> break;
> case MachineOperand::MO_GlobalAddress:
> // FIXME: Make this faster - print as operand will create a slot tracker to
More information about the llvm-commits
mailing list