[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>.
----------------
lhames wrote:
> 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? 
Oh fair, probably. Though traits classes tend to be all public members, so maybe the base template could be a struct too.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/OrcRPCTPCServer.h:130
+public:
+  static const char *getName() { return "ReserveMemResultElement"; }
+};
----------------
lhames wrote:
> 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.
Could have a pointer identity DenseMap trait for StringRefs I guess... *goes to look* slightly surprised we don't have one already. My motivation's mostly the same (but less serious) as raw 'new' - const char*s make me give the code a second look in ways that StringRefs don't.


================
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;
----------------
lhames wrote:
> 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?
> I think we keep it though: As you noticed -- Naming the argument types ends up making the lambda quite long.

Fair

>  I guess we could use auto for this once we move to C++17?

I was going to suggest that the conversion isn't available then (because the call operator becomes a template), but apparently it does work - neat! and it does template argument deduction to instantiate a specific call operator when the conversion is performed. So, yeah, one day.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/OrcRPCTPCServer.h:559-562
+#ifdef __APPLE__
+        if (*Sym != '\0')
+          ++Sym;
+#endif
----------------
lhames wrote:
> dblaikie wrote:
> > What's this about?
> It's the MachO leading-underscore demangling, but it would be better written as `if (*Sym != '_') ...;`. Updated. 
Hmm - it strikes me as odd because it's using a compile-time feature rather than say, an API query to test whether the binary format is MachO. I guess that's because of the context of this being on the machine where the code will execute and thus __APPLE__ is synonymous with MachO at that point? If it were possible to rely on some other property like the type of the object file where the symbol came from, that might make it a bit easier to test portably or less error-prone in some weird future where someone might be testing different object formats, etc?


================
Comment at: llvm/lib/ExecutionEngine/Orc/TPCEHFrameRegistrar.cpp:28-50
+  {
+    std::string RegisterWrapperName, DeregisterWrapperName;
+    if (TPC.getTargetTriple().isOSBinFormatMachO()) {
+      RegisterWrapperName += '_';
+      DeregisterWrapperName += '_';
+    }
+    RegisterWrapperName += "llvm_orc_registerEHFrameSectionWrapper";
----------------
lhames wrote:
> 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.
Ah - I'd be inclined to push back a bit there, given it's a bit uncommon & not sure about other readers, but for me leads me to wonder what novel details are in this code such that a dtor needs to run before the return statement/making this code harder to read. But your call.


================
Comment at: llvm/lib/ExecutionEngine/OrcTargetProcess/TargetExecutionUtils.cpp:39
+
+  return Main(Args.size() + !!ProgramName, ArgV.data());
+}
----------------
lhames wrote:
> 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).
Fair enough (FWIW the numbers skew a bit - not all of the `!!`s are for pointers, some for integer values (so would be comparable to `!= 0`), Optional, etc - and, amusingly two dozen are emphatic comments or assert messages like "Case index out of range!!!" - always 3 in those cases, it seems, I didn't see any !! emphases)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90132