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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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>.
----------------
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")


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/RPC/RPCSerialization.h:599
+      return Err;
+    if (O != None)
+      if (auto Err = serializeSeq(C, *O))
----------------
I'd usually write this as "if (O)" fwiw (especially with the deref on the next line - seems clear enough what's being tested), but all good either way.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/Shared/TargetProcessControlTypes.h:29-31
+  UIntWrite() = default;
+  UIntWrite(JITTargetAddress Address, T Value)
+      : Address(Address), Value(Value) {}
----------------
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? 


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/OrcRPCTPCServer.h:130
+public:
+  static const char *getName() { return "ReserveMemResultElement"; }
+};
----------------
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?


================
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;
----------------
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?


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/OrcRPCTPCServer.h:529
+
+  Expected<tpctypes::DylibHandle> loadDylib(const std::string &Path) {
+    std::string ErrMsg;
----------------
StringRef, perhaps?


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/OrcRPCTPCServer.h:559-562
+#ifdef __APPLE__
+        if (*Sym != '\0')
+          ++Sym;
+#endif
----------------
What's this about?


================
Comment at: llvm/lib/ExecutionEngine/Orc/TPCEHFrameRegistrar.cpp:28-50
+  {
+    std::string RegisterWrapperName, DeregisterWrapperName;
+    if (TPC.getTargetTriple().isOSBinFormatMachO()) {
+      RegisterWrapperName += '_';
+      DeregisterWrapperName += '_';
+    }
+    RegisterWrapperName += "llvm_orc_registerEHFrameSectionWrapper";
----------------
What's this scope for specifically? (I can't quickly spot which dtor you're trying to run before the return statement/make_unique)


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


================
Comment at: llvm/lib/ExecutionEngine/Orc/TargetProcessControl.cpp:102
+                                    ArrayRef<std::string> Args) {
+  using MainTy = int (*)(int, char *[]);
+  return orc::runAsMain(jitTargetAddressToFunction<MainTy>(MainFnAddr), Args);
----------------
Not sure if you want to, but just in case: You can make this actually a function type (Rather than a function pointer type): "using MainTy = int(int, char *[]);" and then have the usage be "jitTargetAddressToFunction<MainTy*>" - not sure if that's better, but an option in case in interests.


================
Comment at: llvm/lib/ExecutionEngine/OrcTargetProcess/TargetExecutionUtils.cpp:39
+
+  return Main(Args.size() + !!ProgramName, ArgV.data());
+}
----------------
Don't see much `!!` in LLVM - maybe (ProgramName != nullptr) instead?


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink-executor/llvm-jitlink-executor.cpp:51
+#ifndef LLVM_ON_UNIX
+  printErrorAndExit("listen option not supported");
+#else
----------------
Not possible to support, or "not implemented yet"?


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink-executor/llvm-jitlink-executor.cpp:90
+  else {
+    StringRef Arg1(argv[1]);
+    StringRef SpecifierType, Specifier;
----------------
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")


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink.cpp:604-605
+    // Close the parent ends of the pipes
+    close(PipeFD[0][1]);
+    close(PipeFD[1][0]);
+
----------------
These get a bit hard to read - would it be worth having two separate names for the PipeFD arrays, maybe even some constants for the 0/1, which end is which sort of thing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90132