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