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

[Duncan P. N. Exon Smith]

> On 2015 Jun 17, at 15:20, Alex L <arphaman at gmail.com> wrote:
> 
> 
> 
> 2015-06-17 15:07 GMT-07:00 Duncan P. N. Exon Smith <dexonsmith at apple.com>:
> 
> > 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?
> 
> Yes, references to unnamed BB aren't preserved ATM.
>  
> 
> 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");

Actually, I take it back.  We probably don't want to crash here.  Let's
follow LLVM IR's model of printing something useful, but preventing it
from round-tripping.  Something like this should work:

    YamlMBB.Name = BB ? (BB->hasName() ? BB->getName() : "<unnamed bb>") : "";

(Feel free to use if/else instead of my ternaries.)

That way if someone is dumping this out they get something reasonable, but
it's extremely unlikely to succeed at parse time.

>     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);