[PATCH] New JIT APIs (ORC)

[Justin Bogner]

Lang Hames <lhames at gmail.com> writes:
> Attached is an updated patch (against r226514) for the Orc JIT APIs that were
> discussed on the llvm-dev list the other day.
>
> The impact on existing code is minimal. A new flag (-use-orc) is added to lli
> to allow the Orc-based MCJITReplacement class to be substituted as the
> execution engine. I expect this will be useful for testing. A new method,
> getSymbolAddressInLogicalDylib, is added to RTDyldMemoryManager, however this
> can be safely ignored by MCJIT users - it's only there to support Orc.
>
> There are no tests yet. I hope to start addressing this soon, but my
> preference would be to get this in tree so everyone can muck around with it
> and contribute tests.
>
> Questions and comments welcome.

This looks really nice. I just have a couple of nitpicks, really.

> +++ include/llvm/ExecutionEngine/ObjectMemoryBuffer.h	(working copy)
 ...
> +class ObjectMemoryBuffer : public MemoryBuffer {
> +public:
> +  template <unsigned N>
> +  ObjectMemoryBuffer(SmallVector<char, N> SV)
> +    : SV(std::move(SV)), BufferName("<in-memory object>") {
> +    init(this->SV.begin(), this->SV.end(), false);
> +  }
> +
> +  template <unsigned N>
> +  ObjectMemoryBuffer(SmallVector<char, N> SV, StringRef Name)
> +    : SV(std::move(SV)), BufferName(Name) {
> +    init(this->SV.begin(), this->SV.end(), false);
> +  }
> +  const char* getBufferIdentifier() const override { return BufferName.c_str(); }
> +
> +  BufferKind getBufferKind() const override { return MemoryBuffer_Malloc; }
> +
> +private:
> +  SmallVector<char, 4096> SV;

The constructors are templated, but the member hardcodes 4096 so the
only valid argument to the templates is 4096. Either template the type
or un-template the constructors?

> +++ include/llvm/ExecutionEngine/Orc/IndirectionUtils.h	(working copy)
 ...
> +/// @brief Persistent name mangling.
> +///
> +///   This class provides name mangling that can outlive a Module (and its
> +/// DataLayout).
> +class PersistentMangler {
> +public:
> +  PersistentMangler(DataLayout DL) : DL(std::move(DL)), M(&this->DL) {}

Shouldn't DL be an rvalue reference here?

> +++ include/llvm/ExecutionEngine/Orc/LazyEmittingLayer.h	(working copy)
 ...
> +    // Build the MangledNames map. Bails out early (with MangledNames set to
> +    // nullptr) if the given SearchName is found while building the map.
> +    void buildMangledNames(StringRef SearchName,
> +                           bool ExportedSymbolsOnly) const {
> +      auto Names = llvm::make_unique<StringMap<bool>>();
> +
> +      for (const auto &M : Ms) {
> +        Mangler Mang(M->getDataLayout());
> +
> +        for (const auto &GV : M->globals())
> +          if (addGlobalValue(*Names, GV, Mang, SearchName, ExportedSymbolsOnly))
> +            return;
> +
> +        for (const auto &F : *M)
> +          if (addGlobalValue(*Names, F, Mang, SearchName, ExportedSymbolsOnly))
> +            return;
> +      }
> +
> +      MangledNames = std::move(Names);
> +    }

This actually leaves MangledNames untouched if it bails out early. I
don't know that it matters much which behaviour we choose, but please
make the comment and the code match.

> +++ lib/ExecutionEngine/Orc/IndirectionUtils.cpp	(working copy)
 ...
> +std::vector<std::unique_ptr<Module>>
> +explode(const Module &OrigMod, const JITIndirections &Indirections) {
> +  std::set<std::string> ImplNames;
> +
> +  for (const auto &FuncName : Indirections.IndirectedNames)
> +    ImplNames.insert(Indirections.GetImplName(FuncName));
> +
> +  return explode(
> +      OrigMod, [&](const Function &F) { return ImplNames.count(F.getName()); });

It's better to sort a vector and binary search here, rather than use a
std::set.