<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">2015-05-21 15:42 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 21, at 15:14, Alex Lorenz <<a href="mailto:arphaman@gmail.com">arphaman@gmail.com</a>> wrote:<br>
><br>
> I've rebased the patch and removed the unnecessary target details from test cases.<br>
><br>
><br>
> REPOSITORY<br>
> rL LLVM<br>
><br>
> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D9841&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=iTEM23v270B3kE4u3t-NBVh7T2QjSKX8Rs-tFd06c5o&s=m6t7MQwZWceqC4ytq4LZljn01xCRq3g2YOnzT3Ln9pE&e=" target="_blank">http://reviews.llvm.org/D9841</a><br>
><br>
> Files:<br>
> lib/CodeGen/MIR/MIRParser.cpp<br>
> lib/CodeGen/MIR/MIRPrinter.cpp<br>
> lib/CodeGen/MIR/MIRPrinter.h<br>
> lib/CodeGen/MIR/MIRPrintingPass.cpp<br>
> lib/CodeGen/MIR/YAMLMapping.h<br>
> test/CodeGen/MIR/mf.mir<br>
> test/CodeGen/MIR/mfNameMissing.mir<br>
<br>
</span>These test names are kind of opaque, and the capitalization will make<br>
it hard to tab-complete them on the command-line.<br>
<br>
I'd prefer:<br>
<br>
MIR/machine-function.mir<br>
MIR/invalid-machine-function-missing-name.mir<br>
<br>
or:<br>
<br>
MIR/machine-function.mir<br>
MIR/machine-function-missing-name.mir<br>
<br>
<br>
Anything along those lines is fine with me.<br>
<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=iTEM23v270B3kE4u3t-NBVh7T2QjSKX8Rs-tFd06c5o&s=cSTTGZnv5e4AH0lYom3ehqyUwL3Q9apzzekqtMFFWmA&e=" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
> <D9841.26280.patch><br>
<br>
> Index: lib/CodeGen/MIR/MIRParser.cpp<br>
> ===================================================================<br>
> --- lib/CodeGen/MIR/MIRParser.cpp<br>
> +++ lib/CodeGen/MIR/MIRParser.cpp<br>
> @@ -13,6 +13,7 @@<br>
> //===----------------------------------------------------------------------===//<br>
><br>
> #include "llvm/CodeGen/MIR/MIRParser.h"<br>
> +#include "YAMLMapping.h"<br>
> #include "llvm/ADT/StringRef.h"<br>
> #include "llvm/ADT/STLExtras.h"<br>
> #include "llvm/AsmParser/Parser.h"<br>
> @@ -38,10 +39,16 @@<br>
> MIRParserImpl(std::unique_ptr<MemoryBuffer> Contents, StringRef Filename,<br>
> LLVMContext &Context);<br>
><br>
> - /// Try to parse the optional LLVM module in the MIR file.<br>
> + /// Try to parse the optional LLVM module and the machine functions in the MIR<br>
> + /// file.<br>
> ///<br>
> - /// Return null if an error occurred while parsing the LLVM module.<br>
> - std::unique_ptr<Module> parseLLVMModule(SMDiagnostic &Error);<br>
> + /// Return null if an error occurred.<br>
> + std::unique_ptr<Module> parse(SMDiagnostic &Error);<br>
> +<br>
> + /// Parse the machine function in the current YAML document.<br>
> + ///<br>
> + /// Return true if an error occurred.<br>
> + bool parseMachineFunction(yaml::Input &In);<br>
> };<br>
><br>
> } // end anonymous namespace<br>
> @@ -52,21 +59,53 @@<br>
> SM.AddNewSourceBuffer(std::move(Contents), SMLoc());<br>
> }<br>
><br>
> -std::unique_ptr<Module> MIRParserImpl::parseLLVMModule(SMDiagnostic &Error) {<br>
> - yaml::Input In(SM.getMemoryBuffer(SM.getMainFileID())->getBuffer());<br>
> +static void handleYAMLDiag(const SMDiagnostic &Diag, void *Context) {<br>
> + *reinterpret_cast<SMDiagnostic *>(Context) = Diag;<br>
> +}<br>
><br>
> - // Parse the block scalar manually so that we can return unique pointer<br>
> - // without having to go trough YAML traits.<br>
> - if (In.setCurrentDocument()) {<br>
> +std::unique_ptr<Module> MIRParserImpl::parse(SMDiagnostic &Error) {<br>
> + yaml::Input In(SM.getMemoryBuffer(SM.getMainFileID())->getBuffer(),<br>
> + /*Ctxt=*/nullptr, handleYAMLDiag, &Error);<br>
<br>
Can you clang-format this? The comment right against the `nullptr` is<br>
awkward.<br></blockquote><div><br></div><div>Clang-format leaves it likes this, should I add a space between the comment</div><div>and nullptr?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +<br>
> + std::unique_ptr<Module> Mod;<br>
<br>
Nitpick: typically I've seen either full words or initializisms; since<br>
the full word is out, probably `M` makes sense here.<br>
<br>
> + bool DocumentStatus = In.setCurrentDocument();<br>
> + if (DocumentStatus) {<br>
<br>
Shouldn't this just return immediately if `false`? Maybe after an<br>
error message?<br>
<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>
> dyn_cast_or_null<yaml::BlockScalarNode>(In.getCurrentNode())) {<br>
<br>
Shouldn't this just return immediately if not `BSN` (maybe after an<br>
error message)? Or does the code that follows somehow subsume this?<br></blockquote><div><br></div><div>No, this should continue even if the current node isn't BSN, since the LLVM IR</div><div>is optional, so the first document doesn't have to be a BSN.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> - return parseAssembly(MemoryBufferRef(BSN->getValue(), Filename), Error,<br>
> - Context);<br>
> + Mod = parseAssembly(MemoryBufferRef(BSN->getValue(), Filename), Error,<br>
> + Context);<br>
> + if (!Mod)<br>
> + return Mod;<br>
> + In.nextDocument();<br>
> }<br>
> }<br>
><br>
> - // Create an new, empty module.<br>
> - return llvm::make_unique<Module>(Filename, Context);<br>
> + bool HasLLVMModule = (bool)Mod;<br>
> + if (!HasLLVMModule) // Create an new, empty module.<br>
> + Mod = llvm::make_unique<Module>(Filename, Context);<br>
> +<br>
> + // Parse the machine functions.<br>
> + if (HasLLVMModule)<br>
> + DocumentStatus = In.setCurrentDocument();<br>
<br>
This logic looks awkward. What about this?<br>
<br>
if (M)<br>
DocumentStatus = In.setCurrentDocument();<br>
else // Create an new, empty module.<br>
M = llvm::make_unique<Module>(Filename, Context);<br>
<br>
// Parse the machine functions.<br>
<br>
But couldn't this all get rolled up with the logic at the top?<br>
<br>
if (const auto *BSN = /* ... */) {<br>
M = parseAsembly(...);<br>
if (!M)<br>
return M;<br>
In.nextDocument();<br>
if (!In.setCurrentDocument())<br>
return M;<br>
} else {<br>
M = llvm::make_unique<Module>(...);<br>
}<br>
<br>
> + for (; DocumentStatus; DocumentStatus = In.setCurrentDocument()) {<br>
<br>
All this `DocumentStatus` control flow is pretty hard to follow. This<br>
one just looks like:<br>
<br>
do { /* ... */ } while (In.setCurrentDocument());<br>
<br>
> + if (parseMachineFunction(In))<br>
> + return nullptr;<br>
> + In.nextDocument();<br>
> + }<br>
> +<br>
> + return Mod;<br>
> +}<br>
> +<br>
> +bool MIRParserImpl::parseMachineFunction(yaml::Input &In) {<br>
> + yaml::MachineFunction MF;<br>
> + yaml::yamlize(In, MF, false);<br>
> + if (In.error())<br>
> + return true;<br>
> + // TODO: Initialize the real machine function with the state in the yaml<br>
> + // machine function later on.<br>
> + return false;<br>
> }<br>
><br>
> std::unique_ptr<Module> llvm::parseMIRFile(StringRef Filename,<br>
> @@ -86,5 +125,5 @@<br>
> LLVMContext &Context) {<br>
> auto Filename = Contents->getBufferIdentifier();<br>
> MIRParserImpl Parser(std::move(Contents), Filename, Context);<br>
> - return Parser.parseLLVMModule(Error);<br>
> + return Parser.parse(Error);<br>
> }<br>
> Index: lib/CodeGen/MIR/MIRPrinter.cpp<br>
> ===================================================================<br>
> --- lib/CodeGen/MIR/MIRPrinter.cpp<br>
> +++ lib/CodeGen/MIR/MIRPrinter.cpp<br>
> @@ -13,8 +13,10 @@<br>
> //===----------------------------------------------------------------------===//<br>
><br>
> #include "MIRPrinter.h"<br>
> +#include "YAMLMapping.h"<br>
> #include "llvm/ADT/STLExtras.h"<br>
> #include "llvm/IR/Module.h"<br>
> +#include "llvm/CodeGen/MachineFunction.h"<br>
> #include "llvm/Support/MemoryBuffer.h"<br>
> #include "llvm/Support/raw_ostream.h"<br>
> #include "llvm/Support/YAMLTraits.h"<br>
> @@ -32,6 +34,8 @@<br>
> MIRPrinter(raw_ostream &OS);<br>
><br>
> void printModule(const Module &Mod);<br>
> +<br>
> + void printFunction(const MachineFunction &MF);<br>
> };<br>
><br>
> } // end anonymous namespace<br>
> @@ -60,7 +64,19 @@<br>
> Out << const_cast<Module &>(Mod);<br>
> }<br>
><br>
> +void MIRPrinter::printFunction(const MachineFunction &MF) {<br>
> + yaml::MachineFunction YamlMF;<br>
> + YamlMF.Name = MF.getName();<br>
> + yaml::Output Out(OS);<br>
> + Out << YamlMF;<br>
> +}<br>
> +<br>
> void llvm::printMIR(raw_ostream &OS, const Module &Mod) {<br>
> MIRPrinter Printer(OS);<br>
> Printer.printModule(Mod);<br>
> }<br>
> +<br>
> +void llvm::printMIR(raw_ostream &OS, const MachineFunction &MF) {<br>
> + MIRPrinter Printer(OS);<br>
> + Printer.printFunction(MF);<br>
> +}<br>
> Index: lib/CodeGen/MIR/MIRPrinter.h<br>
> ===================================================================<br>
> --- lib/CodeGen/MIR/MIRPrinter.h<br>
> +++ lib/CodeGen/MIR/MIRPrinter.h<br>
> @@ -7,23 +7,27 @@<br>
> //<br>
> //===----------------------------------------------------------------------===//<br>
> //<br>
> -// This file declares the function that prints out the LLVM IR using the MIR<br>
> -// serialization format.<br>
> -// TODO: Print out machine functions.<br>
> +// This file declares the functions that print out the LLVM IR and machine<br>
> +// functions using the MIR serialization format.<br>
> //<br>
> //===----------------------------------------------------------------------===//<br>
><br>
> #ifndef LLVM_LIB_CODEGEN_MIR_MIRPRINTER_H<br>
> #define LLVM_LIB_CODEGEN_MIR_MIRPRINTER_H<br>
><br>
> namespace llvm {<br>
><br>
> +class MachineFunction;<br>
> class Module;<br>
> class raw_ostream;<br>
><br>
> /// Print LLVM IR using the MIR serialization format to the given output stream.<br>
> void printMIR(raw_ostream &OS, const Module &Mod);<br>
><br>
> +/// Print a machine function using the MIR serialization format to the given<br>
> +/// output stream.<br>
> +void printMIR(raw_ostream &OS, const MachineFunction &MF);<br>
> +<br>
> } // end namespace llvm<br>
><br>
> #endif<br>
> Index: lib/CodeGen/MIR/MIRPrintingPass.cpp<br>
> ===================================================================<br>
> --- lib/CodeGen/MIR/MIRPrintingPass.cpp<br>
> +++ lib/CodeGen/MIR/MIRPrintingPass.cpp<br>
> @@ -28,6 +28,7 @@<br>
> struct MIRPrintingPass : public MachineFunctionPass {<br>
> static char ID;<br>
> raw_ostream &OS;<br>
> + std::string MachineFunctions;<br>
><br>
> MIRPrintingPass() : MachineFunctionPass(ID), OS(dbgs()) {}<br>
> MIRPrintingPass(raw_ostream &OS) : MachineFunctionPass(ID), OS(OS) {}<br>
> @@ -40,12 +41,16 @@<br>
> }<br>
><br>
> virtual bool runOnMachineFunction(MachineFunction &MF) override {<br>
> - // TODO: Print out the machine function.<br>
> + std::string Str;<br>
> + raw_string_ostream StrOS(Str);<br>
> + printMIR(StrOS, MF);<br>
> + MachineFunctions.append(StrOS.str());<br>
> return false;<br>
> }<br>
><br>
> virtual bool doFinalization(Module &M) override {<br>
> printMIR(OS, M);<br>
> + OS << MachineFunctions;<br>
> return false;<br>
> }<br>
> };<br>
> Index: lib/CodeGen/MIR/YAMLMapping.h<br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ lib/CodeGen/MIR/YAMLMapping.h<br>
> @@ -0,0 +1,37 @@<br>
> +//===- YAMLMapping.h - Describes the mapping between MIR and YAML ---------===//<br>
> +//<br>
> +// The LLVM Compiler Infrastructure<br>
> +//<br>
> +// This file is distributed under the University of Illinois Open Source<br>
> +// License. See LICENSE.TXT for details.<br>
> +//<br>
> +//===----------------------------------------------------------------------===//<br>
> +//<br>
> +// This file implements the mapping between various MIR data structures and<br>
> +// their corresponding YAML representation.<br>
> +//<br>
> +//===----------------------------------------------------------------------===//<br>
> +<br>
> +#ifndef LLVM_LIB_CODEGEN_MIR_YAMLMAPPING_H<br>
> +#define LLVM_LIB_CODEGEN_MIR_YAMLMAPPING_H<br>
> +<br>
> +#include "llvm/ADT/StringRef.h"<br>
> +#include "llvm/Support/YAMLTraits.h"<br>
> +<br>
> +namespace llvm {<br>
> +namespace yaml {<br>
> +<br>
> +struct MachineFunction {<br>
> + StringRef Name;<br>
> +};<br>
> +<br>
> +template <> struct MappingTraits<MachineFunction> {<br>
> + static void mapping(IO &YamlIO, MachineFunction &MF) {<br>
> + YamlIO.mapRequired("name", MF.Name);<br>
> + }<br>
> +};<br>
> +<br>
> +} // end namespace yaml<br>
> +} // end namespace llvm<br>
> +<br>
> +#endif<br>
> Index: test/CodeGen/MIR/mf.mir<br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ test/CodeGen/MIR/mf.mir<br>
> @@ -0,0 +1,24 @@<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 parses machine functions correctly.<br>
> +<br>
> +--- |<br>
> +<br>
> + define i32 @foo() {<br>
> + ret i32 0<br>
> + }<br>
> +<br>
> + define i32 @bar() {<br>
> + ret i32 0<br>
> + }<br>
> +<br>
> +...<br>
> +---<br>
> +# CHECK: name: foo<br>
> +# CHECK-NEXT: ...<br>
> +name: foo<br>
> +...<br>
> +---<br>
> +# CHECK: name: bar<br>
> +# CHECK-NEXT: ...<br>
> +name: bar<br>
> +...<br>
> Index: test/CodeGen/MIR/mfNameMissing.mir<br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ test/CodeGen/MIR/mfNameMissing.mir<br>
> @@ -0,0 +1,22 @@<br>
> +# RUN: not llc -start-after branch-folder -stop-after branch-folder -o /dev/null %s 2>&1 | FileCheck %s<br>
> +# This test ensures that an error is reported when a machine function doesn't<br>
> +# have a name attribute.<br>
> +<br>
> +--- |<br>
> +<br>
> + define i32 @foo() {<br>
> + ret i32 0<br>
> + }<br>
> +<br>
> + define i32 @bar() {<br>
> + ret i32 0<br>
> + }<br>
> +<br>
> +...<br>
> +---<br>
> +# CHECK: [[@LINE+1]]:1: error: missing required key 'name'<br>
> +nme: foo<br>
> +...<br>
> +---<br>
<br>
</blockquote></div><br></div></div>