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

[Stefan Gränitz via Phabricator via llvm-commits]

sgraenitz added a comment.

Overall, I think this looks pretty good! I didn't follow the design discussions closely, so please bare with me in case some of my questions are dumb :-) In a way it makes sure other people will understand your code as well.



================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h:60
+
+  /// Runs previously specified denitialization actions
+  virtual void deinitialize(std::vector<ExecutorAddr> &Allocations,
----------------
Nit: typo `deinitialization`


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h:62
+  virtual void deinitialize(std::vector<ExecutorAddr> &Allocations,
+                            OnDeinitializedFunction OnDeInitialized) = 0;
+
----------------
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?


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h:68
+  virtual void release(std::vector<ExecutorAddr> &Reservations,
+                       OnReleasedFunction OnRelease) = 0;
+
----------------
The base class doesn't deal with processes. And `release()` is the inverse operation for `reserve()` right? Then we might want the comment to reflect that and say something like `Release address space in the executor process`.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h:77
+
+  using MemoryMapper::reserve;
+  void reserve(size_t NumBytes, OnReservedFunction OnReserved) override;
----------------
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.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h:91
+  void release(std::vector<ExecutorAddr> &Reservations,
+               OnReleasedFunction OnRelease) override;
+
----------------
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.


================
Comment at: llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp:50
+    std::memset(
+        (AI.MappingBase + Segment.Offset + Segment.ContentSize).toPtr<void *>(),
+        0, Segment.ZeroFillSize);
----------------
`Base = AI.MappingBase + Segment.Offset`


================
Comment at: llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp:68
+    Allocations[MinAddr].DeinitializationActions =
+        std::move(*DeinitializeActions);
+  }
----------------
Can we add a comment that explains the special role of `MinAddr` in regard to allocation actions here? Thanks


================
Comment at: llvm/unittests/ExecutionEngine/Orc/MemoryMapperTest.cpp:144
+    std::vector<ExecutorAddr> DeinitAddr = {*Init1};
+    EXPECT_THAT_ERROR(deinitialize(*Mapper, DeinitAddr), Succeeded());
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127491