[PATCH] MIR Serialization: Serialize MBB Successors.

[Alex L]

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.

Alex

2015-06-29 10:37 GMT-07:00 Duncan P. N. Exon Smith <dexonsmith at apple.com>:

>
> > 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150629/6e7ce9af/attachment.html>