[PATCH] MIR Serialization: Print and parse function's MBBs and simple MBB attributes.

[Duncan P. N. Exon Smith]

> On 2015 Jun 15, at 17:05, Alex Lorenz <arphaman at gmail.com> wrote:
> 
> Hi dexonsmith, bob.wilson, bogner,
> 
> This patch implements the initial serialization of the machine basic blocks in a machine function. Only the simple, scalar MBB attributes are serialized. The link to LLVM IR's basic block is preserved when that basic block has a name.
> 
> REPOSITORY
>  rL LLVM
> 
> http://reviews.llvm.org/D10465
> 
> Files:
>  include/llvm/CodeGen/MIRYamlMapping.h
>  lib/CodeGen/MIRParser/MIRParser.cpp
>  lib/CodeGen/MIRPrinter.cpp
>  test/CodeGen/MIR/basic-blocks.mir
> 
> EMAIL PREFERENCES
>  http://reviews.llvm.org/settings/panel/emailpreferences/
> <D10465.27730.patch>

> Index: lib/CodeGen/MIRParser/MIRParser.cpp
> ===================================================================
> --- lib/CodeGen/MIRParser/MIRParser.cpp
> +++ lib/CodeGen/MIRParser/MIRParser.cpp
> @@ -184,6 +191,14 @@
>    new UnreachableInst(Context, BB);
>  }
>  
> +static const BasicBlock *findBasicBlock(const Function *Fn, StringRef Name) {

Can you take `Fn` by reference?

> +  for (const BasicBlock &BB : Fn->getBasicBlockList()) {
> +    if (Name == BB.getName())
> +      return &BB;

Seems expensive.  Aren't basic blocks stored in `Function`'s symbol
table?  If so, the whole function should be:

    return dyn_cast_or_null<BasicBlock>(Fn.getValueSymbolTable().lookup(Name)));

and maybe you don't need to separate this out at all (although it's fine
if you think it's cleaner).

> +  }
> +  return nullptr;
> +}
> +
>  bool MIRParserImpl::initializeMachineFunction(MachineFunction &MF) {
>    auto It = Functions.find(MF.getName());
>    if (It == Functions.end())
> Index: lib/CodeGen/MIRPrinter.cpp
> ===================================================================
> --- lib/CodeGen/MIRPrinter.cpp
> +++ lib/CodeGen/MIRPrinter.cpp
> @@ -61,10 +64,27 @@
>    YamlMF.Alignment = MF.getAlignment();
>    YamlMF.ExposesReturnsTwice = MF.exposesReturnsTwice();
>    YamlMF.HasInlineAsm = MF.hasInlineAsm();
> +  for (const auto &MBB : MF) {
> +    yaml::MachineBasicBlock YamlMBB;
> +    convert(YamlMBB, MBB);
> +    YamlMF.BasicBlocks.push_back(YamlMBB);
> +  }
>    yaml::Output Out(OS);
>    Out << YamlMF;
>  }
>  
> +void MIRPrinter::convert(yaml::MachineBasicBlock &YamlMBB,
> +                         const MachineBasicBlock &MBB) {
> +  const auto *BB = MBB.getBasicBlock();
> +  if (BB && BB->hasName())
> +    YamlMBB.Name = BB->getName();
> +  else
> +    YamlMBB.Name = "";

What if BB doesn't have a name?  I guess it doesn't get serialized yet?

That's fine as a temporary missing feature, but I think you should
phrase it this way:

    // FIXME: Track local value slots in functions so we can serialize
    // machine basic blocks derived from unnamed LLVM IR basic blocks.
    assert((!BB || BB->hasName()) &&
           "No serialization support yet for unnamed basic blocks");
    YamlMBB.Name = BB ? BB->getName() : "";

> +  YamlMBB.Alignment = MBB.getAlignment();
> +  YamlMBB.AddressTaken = MBB.hasAddressTaken();
> +  YamlMBB.IsLandingPad = MBB.isLandingPad();
> +}
> +
>  void llvm::printMIR(raw_ostream &OS, const Module &M) {
>    yaml::Output Out(OS);
>    Out << const_cast<Module &>(M);