[PATCH] MIR Serialization: Serialize MBB Successors.

[Duncan P. N. Exon Smith]

> 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