[PATCH] D90132: [ORC] Prototype ORC library reorg + RPC based TargetProcessControl.

[Lang Hames via Phabricator via llvm-commits]

lhames added inline comments.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/RPC/RPCSerialization.h:593-594
+template <typename ChannelT, typename T>
+class SerializationTraits<ChannelT, Optional<T>> {
+public:
+  /// Serialize an Optional<T>.
----------------
dblaikie wrote:
> I'd probably write anything with all-public members as "struct" (don't think we have style policy on this - and certainly some folks/styles reserve "struct" for "type with only member variables/no invariants")
This is a specialization and the base case is a class, so I think the specialization should be (and perhaps must be) a class? 


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/Shared/TargetProcessControlTypes.h:29-31
+  UIntWrite() = default;
+  UIntWrite(JITTargetAddress Address, T Value)
+      : Address(Address), Value(Value) {}
----------------
dblaikie wrote:
> Could potentially skip these and use braced init whenever you want to create an object of this type and initialize the members in one go? (similarly below/for other similar types) Or perhaps there are difficult issues with deduced types and such? 
Yeah. I'm still conservative with my braced inits as I've seen them fail on MSVC builders but don't have a good sense of what those compilers do/don't support. Ideally this can be removed, but we can try fixing that selectively after the patch lands so that any failures due to it are clear.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/Shared/TargetProcessControlTypes.h:110
+  WrapperFunctionResult(WrapperFunctionResult &&Other)
+      : R({0, {.ValuePtr = nullptr}, nullptr}) {
+    std::swap(R, Other.R);
----------------
sgraenitz wrote:
> I am getting compile warnings here too (Apple clang-1100.0.33.12). On my machine it says "designated initializers are a C99 feature" :-) One more in OrcRPCTPCServer.h
Thanks for catching this. It should be fixed in the latest patch.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/OrcRPCTPCServer.h:130
+public:
+  static const char *getName() { return "ReserveMemResultElement"; }
+};
----------------
dblaikie wrote:
> Probably use StringRef for these return results - so the length is computed off the string literal, rather than a StringRef/other string conversion happening later and requiring a null terminator search at that point?
I think we want to stick with char*: We use the strings for diagnostics and RPC-handshakes, but neither of those are performance sensitive. The only performance sensitive use case for these strings is as densemap keys,  where using StringRefs would cause us to use value comparisons instead of pointer comparisons.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/OrcRPCTPCServer.h:260
+            C.readBytes(reinterpret_cast<char *>(R.Data.ValuePtr), R.Size)) {
+      R.Destroy = tpctypes::WrapperFunctionResult::destroyWithDeleteArray;
+      return Err;
----------------
dblaikie wrote:
> Would it be simpler to write this as a local stateless lambda (they can be converted to raw function pointers anyway) - I see it's used in two places, but the code is so simple (maybe the type parameters are complicated?) might not be pulling its weight as a named function?
Oh -- I didn't realize that stateless lambdas were convertible to raw function types. That's neat!

I think we keep it though: As you noticed -- Naming the argument types ends up making the lambda quite long. I guess we could use auto for this once we move to C++17?


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/OrcRPCTPCServer.h:529
+
+  Expected<tpctypes::DylibHandle> loadDylib(const std::string &Path) {
+    std::string ErrMsg;
----------------
dblaikie wrote:
> StringRef, perhaps?
StringRefs aren't safely convertible to a c_str -- We'd need to copy to a std::string anyway.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/OrcRPCTPCServer.h:559-562
+#ifdef __APPLE__
+        if (*Sym != '\0')
+          ++Sym;
+#endif
----------------
dblaikie wrote:
> What's this about?
It's the MachO leading-underscore demangling, but it would be better written as `if (*Sym != '_') ...;`. Updated. 


================
Comment at: llvm/lib/ExecutionEngine/Orc/TPCEHFrameRegistrar.cpp:28-50
+  {
+    std::string RegisterWrapperName, DeregisterWrapperName;
+    if (TPC.getTargetTriple().isOSBinFormatMachO()) {
+      RegisterWrapperName += '_';
+      DeregisterWrapperName += '_';
+    }
+    RegisterWrapperName += "llvm_orc_registerEHFrameSectionWrapper";
----------------
dblaikie wrote:
> What's this scope for specifically? (I can't quickly spot which dtor you're trying to run before the return statement/make_unique)
I often scope parts of functions that perform a specific, isolated task (in this case finding (De)RegisterEHFrameWrapperFnAddr. It acts as a porto-function that's easy to yank out if it grows / becomes complex enough, and it ensures that I don't accidentally re-use temporaries that support that task outside the scope.


================
Comment at: llvm/lib/ExecutionEngine/Orc/TPCEHFrameRegistrar.cpp:74
+  BinaryStreamWriter ArgWriter(
+      MutableArrayRef<uint8_t>(ArgBuffer, ArgBufferSize),
+      support::endianness::big);
----------------
dblaikie wrote:
> Maybe add a `makeMutableArrayRef` that takes an array reference and deduces the size from that, rather than needing to specify it explicitly?
Good idea, but I think I'd introduce that in a separate patch.


================
Comment at: llvm/lib/ExecutionEngine/OrcTargetProcess/TargetExecutionUtils.cpp:39
+
+  return Main(Args.size() + !!ProgramName, ArgV.data());
+}
----------------
dblaikie wrote:
> Don't see much `!!` in LLVM - maybe (ProgramName != nullptr) instead?
  % grep -iR '!!' include lib  | wc -l
     155
  % grep -iR '!= nullptr' include lib  | wc -l
     658

Less common, but common enough -- I think this one can stay (or we should come up with a style guide point to dictate the preferred style).


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink-executor/llvm-jitlink-executor.cpp:51
+#ifndef LLVM_ON_UNIX
+  printErrorAndExit("listen option not supported");
+#else
----------------
dblaikie wrote:
> Not possible to support, or "not implemented yet"?
Not implemented yet. We just need a windows implementation. I've added a FIXME.


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink-executor/llvm-jitlink-executor.cpp:90
+  else {
+    StringRef Arg1(argv[1]);
+    StringRef SpecifierType, Specifier;
----------------
dblaikie wrote:
> Generally favor the implicit conversion (`StringRef Arg1 = argv[1]`) - means only implicit conversions can occur, making the code easier to read (because you don't have to be worried that some explicit conversion is occurring - I find this particularly important with unique_ptrs (`unique_ptr<T> u(v())` is "oh, I need to check if this is taking ownership of a raw pointer" versus `unique_ptr<T> u = v()` "oh, great, it's returning unique_ptr by value")
Fixed.


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink.cpp:625
+    char *const Args[] = {ExecutorPath.get(), FDSpecifier.get(), nullptr};
+    int RC = execvp(ExecutorPath.get(), Args);
+    if (RC != 0) {
----------------
sgraenitz wrote:
> What is the advantage of using `execvp` over `execv`? (The latter is what lli did.) I think the way it is now, the default "llvm-jitlink-executor" will only be found if the directory is in PATH.
> 
> Changing this to `execv` I can run a hello world example with the default executor setting once I built both binaries. With `execvp` the default fails, but this still works:
> ```
> ./llvm-jitlink -oop-executor=./llvm-jitlink-executor ../hello_world.o
> ```
I like execvp, since it means that lookup for the executor program is going to follow the same paths as the lookup for llvm-jitlink itself. Maybe the best option would be to use execvp but default to a full path to llvm-jitlink-executor?

I'd be inclined to update lli to match this logic.

I don't have strong feelings about this though. If most people find execv preferable then I am happy to change it. 


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink.cpp:627
+    if (RC != 0) {
+      errs() << "unable to launch out-of-process executor\n";
+      exit(1);
----------------
sgraenitz wrote:
> Maybe this could be:
> ```
>       errs() << "unable to launch out-of-process executor: "
>              << ExecutorPath.get() << "\n";
> ```
Good call. Fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90132/new/

https://reviews.llvm.org/D90132