<div dir="ltr">Thanks, I've cleaned this patch up and made the parsing interfaces more consistent. I also extracted 3 other NFC commits that I can pre-commit before this one.<div><br></div><div>Alex</div></div><div class="gmail_extra"><br><div class="gmail_quote">2015-06-29 10:37 GMT-07:00 Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
> On 2015-Jun-29, at 10:20, Alex Lorenz <<a href="mailto:arphaman@gmail.com">arphaman@gmail.com</a>> wrote:<br>
><br>
> I rebased this patch on top of trunk.<br>
><br>
><br>
> REPOSITORY<br>
> rL LLVM<br>
><br>
> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10699&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=oKx9fMM2Iy8PHf0_-UKQH64LMUph7M7FHI0-lcH8Siw&s=BatoztdgXa4HNN7HifoZfkxosBNCo_GWgITAaBXEfB4&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D10699</a><br>
><br>
> Files:<br>
> include/llvm/CodeGen/MIRYamlMapping.h<br>
> lib/CodeGen/MIRParser/MIParser.cpp<br>
> lib/CodeGen/MIRParser/MIParser.h<br>
> lib/CodeGen/MIRParser/MIRParser.cpp<br>
> lib/CodeGen/MIRPrinter.cpp<br>
> test/CodeGen/MIR/expected-eof-after-successor-mbb.mir<br>
> test/CodeGen/MIR/expected-mbb-reference-for-successor-mbb.mir<br>
> test/CodeGen/MIR/successor-basic-blocks.mir<br>
><br>
> EMAIL PREFERENCES<br>
> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_settings_panel_emailpreferences_&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=oKx9fMM2Iy8PHf0_-UKQH64LMUph7M7FHI0-lcH8Siw&s=spHJKa9TMbgjnFrgBlQCeixmjh-CTvpnQ0Z3waLpWsI&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
</div></div>> <D10699.28684.patch><br>
<br>
> Index: lib/CodeGen/MIRParser/MIParser.cpp<br>
> ===================================================================<br>
> --- lib/CodeGen/MIRParser/MIParser.cpp<br>
> +++ lib/CodeGen/MIRParser/MIParser.cpp<br>
> @@ -64,10 +64,12 @@<br>
> bool error(StringRef::iterator Loc, const Twine &Msg);<br>
><br>
> MachineInstr *parse();<br>
> + MachineBasicBlock *parseMBB();<br>
<br>
Strange to break the pattern here. Actually, strange that<br>
`MachineInstr` doesn't return `bool`, too.<br>
<br>
More below.<br>
<br>
><br>
> bool parseRegister(unsigned &Reg);<br>
> bool parseRegisterOperand(MachineOperand &Dest, bool IsDef = false);<br>
> bool parseImmediateOperand(MachineOperand &Dest);<br>
> + MachineBasicBlock *parseMBBReference();<br>
<br>
Strange to break the pattern here as well.<br>
<br>
> bool parseMBBOperand(MachineOperand &Dest);<br>
> bool parseGlobalAddressOperand(MachineOperand &Dest);<br>
> bool parseMachineOperand(MachineOperand &Dest);<br>
> Index: lib/CodeGen/MIRParser/MIRParser.cpp<br>
> ===================================================================<br>
> --- lib/CodeGen/MIRParser/MIRParser.cpp<br>
> +++ lib/CodeGen/MIRParser/MIRParser.cpp<br>
> @@ -262,9 +262,21 @@<br>
> if (YamlMBB.AddressTaken)<br>
> MBB.setHasAddressTaken();<br>
> MBB.setIsLandingPad(YamlMBB.IsLandingPad);<br>
> + SMDiagnostic Error;<br>
> + // Parse the successors.<br>
> + for (const auto &MBBSource : YamlMBB.Successors) {<br>
> + if (auto *SuccMBB = parseMBBReference(SM, MF, MBBSource.Value, MBBSlots,<br>
> + IRSlots, Error)) {<br>
> + // TODO: Report an error when the successor is self.<br>
> + // TODO: Report an error when adding the same successor more than once.<br>
> + MBB.addSuccessor(SuccMBB);<br>
> + continue;<br>
> + }<br>
> + reportDiagnostic(diagFromMIStringDiag(Error, MBBSource.SourceRange));<br>
> + return true;<br>
<br>
IIRC, previously you've been using this pattern:<br>
<br>
MachineBasicBlock *SuccMBB;<br>
if (parseMBBReference(SuccMBB, SM, MF, MBBSource.Value, MBBSlots, IRSlots, Error))<br>
return error(Error, MBBSource.SourceRange);<br>
<br>
// TODO: Report an error when the successor is self.<br>
// TODO: Report an error when adding the same successor more than once.<br>
MBB.addSuccessor(SuccMBB);<br>
<br>
This helps prevent nesting for the error-free control flow, which is a<br>
real concern given that your TODOs list more errors you're going to have<br>
to add. Besides, patterns are more valuable if you're consistent.<br>
<br>
Is there a good reason to do it differently here?<br>
<br>
If there isn't, I just noticed that `parse()` (which returns a<br>
`MachineInstr*`) is already breaking the pattern. Unless you have a<br>
good reason for that please change it to returning `bool` and taking a<br>
`MachineInstr*&` as well.<br>
<br>
Note: I've also packed the `reportDiagnostic()`,<br>
`diagFromMIStringDig()`, and `return true` together into a function<br>
called `error()`. Kind of a separate issue, but I think you should find<br>
a way to do something like this. Seems like a lot of boilerplate.<br>
<br>
> + }<br>
> // Parse the instructions.<br>
> for (const auto &MISource : YamlMBB.Instructions) {<br>
> - SMDiagnostic Error;<br>
> if (auto *MI = parseMachineInstr(SM, MF, MISource.Value, MBBSlots, IRSlots,<br>
> Error)) {<br>
> MBB.insert(MBB.end(), MI);<br>
> Index: lib/CodeGen/MIRPrinter.cpp<br>
> ===================================================================<br>
> --- lib/CodeGen/MIRPrinter.cpp<br>
> +++ lib/CodeGen/MIRPrinter.cpp<br>
> @@ -193,6 +200,14 @@<br>
> llvm_unreachable("Can't print this kind of register yet");<br>
> }<br>
><br>
> +void MIPrinter::printMBBReference(const MachineBasicBlock &MBB) {<br>
> + OS << "%bb." << MBB.getNumber();<br>
> + if (const auto *BB = MBB.getBasicBlock()) {<br>
> + if (BB->hasName())<br>
> + OS << '.' << BB->getName();<br>
> + }<br>
> +}<br>
> +<br>
> void MIPrinter::print(const MachineOperand &Op, const TargetRegisterInfo *TRI) {<br>
> switch (Op.getType()) {<br>
> case MachineOperand::MO_Register:<br>
> @@ -204,11 +219,7 @@<br>
> OS << Op.getImm();<br>
> break;<br>
> case MachineOperand::MO_MachineBasicBlock:<br>
> - OS << "%bb." << Op.getMBB()->getNumber();<br>
> - if (const auto *BB = Op.getMBB()->getBasicBlock()) {<br>
> - if (BB->hasName())<br>
> - OS << '.' << BB->getName();<br>
> - }<br>
> + printMBBReference(*Op.getMBB());<br>
<br>
This part of the commit -- splitting out a `printMBBReference()`<br>
function -- looks like NFC. Please commit this (and anything else<br>
that's not changing behaviour) separately. Otherwise, it's hard to tell<br>
what's a real change and what isn't.<br>
<br>
> break;<br>
> case MachineOperand::MO_GlobalAddress:<br>
> // FIXME: Make this faster - print as operand will create a slot tracker to<br>
<br>
</blockquote></div><br></div>