[PATCH] D127491: [JITLink][Orc] Add MemoryMapper interface with InProcess implementation

[Anubhab Ghosh via Phabricator via llvm-commits]

argentite marked an inline comment as not done.
argentite added inline comments.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h:62
+  virtual void deinitialize(std::vector<ExecutorAddr> &Allocations,
+                            OnDeinitializedFunction OnDeInitialized) = 0;
+
----------------
sgraenitz wrote:
> In which case do we have multiple `ExecutorAddr` here? `initialize()` only returns a single one -- can it run multiple times for a single MemoryMapper? If this is the case, it may deserve a test case and/or a comment.
> 
> If we keep the `std::vector` I guess it should be passed as `const std::vector<ExecutorAddr> &` -- or is there a design reason for mutability here?
As I understand the a single MemoryMapper should live for the whole duration of JITLink use. When terminating, we can pass the results of multiple initialize to a single deinitialize to save RPC overhead. I have made the vectors const for both deinitialize and release.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h:77
+
+  using MemoryMapper::reserve;
+  void reserve(size_t NumBytes, OnReservedFunction OnReserved) override;
----------------
sgraenitz wrote:
> What is the reason for the `using MemoryMapper::...` statements when we only have one base class? My first thought was that it's a hint for another overload in the base class, but it doesn't seem to be the case.
Ah there were synchronous overloads that we moved to unit test. I forgot to delete them.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h:91
+  void release(std::vector<ExecutorAddr> &Reservations,
+               OnReleasedFunction OnRelease) override;
+
----------------
sgraenitz wrote:
> Maybe we can add a comment for implict vs. explicit deinit/release somewhere? This seems to be specific for the `InProcessMemoryMapper`. It took me a while to figure out that the destructor reimplements them (instead of calling) for the implicit case.
The plan is to have some kind of implicit deinitialization in all MemoryMappers. The release() and deinitialize() are there for cases where we want to release early without terminating the program. Do you think anything specific that we should mention?


================
Comment at: llvm/unittests/ExecutionEngine/Orc/MemoryMapperTest.cpp:144
+    std::vector<ExecutorAddr> DeinitAddr = {*Init1};
+    EXPECT_THAT_ERROR(deinitialize(*Mapper, DeinitAddr), Succeeded());
+
----------------
sgraenitz wrote:
> Alloc1 is testing: explicit deinit + implicit release
> Alloc2 is testing: implicit deinit + implicit release
> 
> I think they are both good to have. However, the explicit release case has no coverage yet.
I have added a test for explicit deinit + explicit release but note that explicit release does not implicitly deinitialize because allocations are not directly tied to a reservation. (Should it?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127491