<div dir="ltr"><br><div class="gmail_extra">Thanks, I committed the patch with your suggestions applied in r239778.</div><div class="gmail_extra"><br></div><div class="gmail_extra">Alex.</div><div class="gmail_extra"><br><div class="gmail_quote">2015-05-29 12:06 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"><span class=""><br>
> On 2015-May-29, at 11:33, Alex Lorenz <<a href="mailto:arphaman@gmail.com">arphaman@gmail.com</a>> wrote:<br>
><br>
> Hi dexonsmith, bogner, bob.wilson,<br>
><br>
> This patch is based on the patch that connects MIRParser to the machine function analysis pass (<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D9928&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=lRMW0S3zJqZzlXAh3nRy3jLjzgS7rMqEdm_k77p6Y8s&s=rt3claKnU78YQiQcWTnY8Fo2kD5sgV10ct6uhdhRkDw&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D9928</a>).<br>
><br>
> This patch creates dummy LLVM functions with one basic block and a return instruction for the parsed machine functions when the MIR file doesn't include LLVM IR.<br>
> This is needed as the machine function analysis pass creates machine functions only for the functions defined in the current LLVM module.<br>
><br>
> REPOSITORY<br>
> rL LLVM<br>
><br>
> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10135&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=lRMW0S3zJqZzlXAh3nRy3jLjzgS7rMqEdm_k77p6Y8s&s=AGDsGnvJUhJ2lfyxme6PZyHkLC9bly7SDO8zHULzKoY&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D10135</a><br>
><br>
> Files:<br>
> lib/CodeGen/MIRParser/MIRParser.cpp<br>
> test/CodeGen/MIR/llvmIRMissing.mir<br>
><br>
</span>I'm skeptical that testcases without backing LLVM IR are going to prove<br>
useful. This particular patch doesn't seem to be too complicated, so<br>
I'm fine with it, but I think you should focus on serializing MIR that<br>
*has* matching LLVM IR.<br>
<br>
A few comments below; otherwise LGTM.<br>
<span class=""><br>
> Index: lib/CodeGen/MIRParser/MIRParser.cpp<br>
> ===================================================================<br>
> --- lib/CodeGen/MIRParser/MIRParser.cpp<br>
> +++ lib/CodeGen/MIRParser/MIRParser.cpp<br>
> @@ -19,6 +19,7 @@<br>
> #include "llvm/AsmParser/Parser.h"<br>
> #include "llvm/CodeGen/MachineFunction.h"<br>
> #include "llvm/CodeGen/MIRYamlMapping.h"<br>
> +#include "llvm/IR/Instructions.h"<br>
> #include "llvm/IR/Module.h"<br>
> #include "llvm/Support/SMLoc.h"<br>
> #include "llvm/Support/SourceMgr.h"<br>
> @@ -51,13 +52,17 @@<br>
> /// Parse the machine function in the current YAML document.<br>
> ///<br>
> /// Return true if an error occurred.<br>
<br>
</span>You should document the effect that `NoLLVMIR` has.<br>
<div><div class="h5"><br>
> - bool parseMachineFunction(yaml::Input &In);<br>
> + bool parseMachineFunction(yaml::Input &In, Module &M, bool NoLLVMIR);<br>
><br>
> /// Initialize the machine function to the state that's described in the MIR<br>
> /// file.<br>
> ///<br>
> /// Return true if error occurred.<br>
> bool initializeMachineFunction(MachineFunction &MF, SMDiagnostic &Error);<br>
> +<br>
> +private:<br>
> + /// Create an empty function with the given name.<br>
> + void createDummyFunction(StringRef Name, Module &M);<br>
> };<br>
><br>
> } // end namespace llvm<br>
> @@ -84,6 +89,7 @@<br>
> }<br>
><br>
> std::unique_ptr<Module> M;<br>
> + bool NoLLVMIR = false;<br>
> // Parse the block scalar manually so that we can return unique pointer<br>
> // without having to go trough YAML traits.<br>
> if (const auto *BSN =<br>
> @@ -98,28 +104,40 @@<br>
> } else {<br>
> // Create an new, empty module.<br>
> M = llvm::make_unique<Module>(Filename, Context);<br>
> + NoLLVMIR = true;<br>
> }<br>
><br>
> // Parse the machine functions.<br>
> do {<br>
> - if (parseMachineFunction(In))<br>
> + if (parseMachineFunction(In, *M, NoLLVMIR))<br>
> return nullptr;<br>
> In.nextDocument();<br>
> } while (In.setCurrentDocument());<br>
><br>
> return M;<br>
> }<br>
><br>
> -bool MIRParserImpl::parseMachineFunction(yaml::Input &In) {<br>
> +bool MIRParserImpl::parseMachineFunction(yaml::Input &In, Module &M,<br>
> + bool NoLLVMIR) {<br>
> auto MF = llvm::make_unique<yaml::MachineFunction>();<br>
> yaml::yamlize(In, *MF, false);<br>
> if (In.error())<br>
> return true;<br>
> auto FunctionName = MF->Name;<br>
> Functions.insert(std::make_pair(FunctionName, std::move(MF)));<br>
> + if (NoLLVMIR)<br>
> + createDummyFunction(FunctionName, M);<br>
> return false;<br>
> }<br>
><br>
> +void MIRParserImpl::createDummyFunction(StringRef Name, Module &M) {<br>
> + auto &Context = M.getContext();<br>
> + Function *F = cast<Function>(M.getOrInsertFunction(<br>
> + Name, FunctionType::get(Type::getVoidTy(Context), false)));<br>
> + BasicBlock *BB = BasicBlock::Create(Context, "Entry", F);<br>
> + ReturnInst::Create(Context, nullptr, BB);<br>
<br>
</div></div>`unreachable` is a simpler terminator than `ret void`, so I think it<br>
makes more sense in a dummy function.</blockquote><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>
> +}<br>
> +<br>
> bool MIRParserImpl::initializeMachineFunction(MachineFunction &MF,<br>
> SMDiagnostic &Error) {<br>
> auto It = Functions.find(MF.getName());<br>
> Index: test/CodeGen/MIR/llvmIRMissing.mir<br>
> ===================================================================<br>
> --- test/CodeGen/MIR/llvmIRMissing.mir<br>
> +++ test/CodeGen/MIR/llvmIRMissing.mir<br>
> @@ -1,5 +1,7 @@<br>
> -# RUN: llc -start-after branch-folder -stop-after branch-folder -o /dev/null %s<br>
> +# RUN: llc -start-after branch-folder -stop-after branch-folder -o /dev/null %s | FileCheck %s<br>
> # This test ensures that the MIR parser accepts files without the LLVM IR.<br>
><br>
> ---<br>
> +# CHECK: name: foo<br>
> +name: foo<br>
> ...<br>
><br>
<br>
<br>
</div></div></blockquote></div><br></div></div>