<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">2015-06-17 15:07 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 15, at 17:05, Alex Lorenz <<a href="mailto:arphaman@gmail.com">arphaman@gmail.com</a>> wrote:<br>
><br>
> Hi dexonsmith, bob.wilson, bogner,<br>
><br>
> 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.<br>
><br>
> REPOSITORY<br>
>  rL LLVM<br>
><br>
> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10465&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=dJtGO2pKCr6fc_OMZmkDh2pwJQnT1yeujpUuRHgukaA&s=D7eM-MkjK-wYA9MOLw9RW4PikAl9t8wGu-tgXzKQKZ8&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D10465</a><br>
><br>
> Files:<br>
>  include/llvm/CodeGen/MIRYamlMapping.h<br>
>  lib/CodeGen/MIRParser/MIRParser.cpp<br>
>  lib/CodeGen/MIRPrinter.cpp<br>
>  test/CodeGen/MIR/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=dJtGO2pKCr6fc_OMZmkDh2pwJQnT1yeujpUuRHgukaA&s=GZr9DoKLX-irRqbJe6g6OExypLsJeJlGBdxyTOOdjEY&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
</div></div>> <D10465.27730.patch><br>
<br>
> Index: lib/CodeGen/MIRParser/MIRParser.cpp<br>
> ===================================================================<br>
> --- lib/CodeGen/MIRParser/MIRParser.cpp<br>
> +++ lib/CodeGen/MIRParser/MIRParser.cpp<br>
> @@ -184,6 +191,14 @@<br>
>    new UnreachableInst(Context, BB);<br>
>  }<br>
><br>
> +static const BasicBlock *findBasicBlock(const Function *Fn, StringRef Name) {<br>
<br>
Can you take `Fn` by reference?<br>
<br>
> +  for (const BasicBlock &BB : Fn->getBasicBlockList()) {<br>
> +    if (Name == BB.getName())<br>
> +      return &BB;<br>
<br>
Seems expensive.  Aren't basic blocks stored in `Function`'s symbol<br>
table?  If so, the whole function should be:<br>
<br>
    return dyn_cast_or_null<BasicBlock>(Fn.getValueSymbolTable().lookup(Name)));<br>
<br>
and maybe you don't need to separate this out at all (although it's fine<br>
if you think it's cleaner).<br>
<br>
> +  }<br>
> +  return nullptr;<br>
> +}<br>
> +<br>
>  bool MIRParserImpl::initializeMachineFunction(MachineFunction &MF) {<br>
>    auto It = Functions.find(MF.getName());<br>
>    if (It == Functions.end())<br>
> Index: lib/CodeGen/MIRPrinter.cpp<br>
> ===================================================================<br>
> --- lib/CodeGen/MIRPrinter.cpp<br>
> +++ lib/CodeGen/MIRPrinter.cpp<br>
> @@ -61,10 +64,27 @@<br>
>    YamlMF.Alignment = MF.getAlignment();<br>
>    YamlMF.ExposesReturnsTwice = MF.exposesReturnsTwice();<br>
>    YamlMF.HasInlineAsm = MF.hasInlineAsm();<br>
> +  for (const auto &MBB : MF) {<br>
> +    yaml::MachineBasicBlock YamlMBB;<br>
> +    convert(YamlMBB, MBB);<br>
> +    YamlMF.BasicBlocks.push_back(YamlMBB);<br>
> +  }<br>
>    yaml::Output Out(OS);<br>
>    Out << YamlMF;<br>
>  }<br>
><br>
> +void MIRPrinter::convert(yaml::MachineBasicBlock &YamlMBB,<br>
> +                         const MachineBasicBlock &MBB) {<br>
> +  const auto *BB = MBB.getBasicBlock();<br>
> +  if (BB && BB->hasName())<br>
> +    YamlMBB.Name = BB->getName();<br>
> +  else<br>
> +    YamlMBB.Name = "";<br>
<br>
What if BB doesn't have a name?  I guess it doesn't get serialized yet?<br></blockquote><div><br></div><div>Yes, references to unnamed BB aren't preserved ATM.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
That's fine as a temporary missing feature, but I think you should<br>
phrase it this way:<br>
<br>
    // FIXME: Track local value slots in functions so we can serialize<br>
    // machine basic blocks derived from unnamed LLVM IR basic blocks.<br>
    assert((!BB || BB->hasName()) &&<br>
           "No serialization support yet for unnamed basic blocks");<br>
    YamlMBB.Name = BB ? BB->getName() : "";</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +  YamlMBB.Alignment = MBB.getAlignment();<br>
> +  YamlMBB.AddressTaken = MBB.hasAddressTaken();<br>
> +  YamlMBB.IsLandingPad = MBB.isLandingPad();<br>
> +}<br>
> +<br>
>  void llvm::printMIR(raw_ostream &OS, const Module &M) {<br>
>    yaml::Output Out(OS);<br>
>    Out << const_cast<Module &>(M);<br>
<br>
</blockquote></div><br></div></div>