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

[Lang Hames via Phabricator via llvm-commits]

lhames added reviewers: sgraenitz, lhames, v.g.vassilev.
lhames added inline comments.
Herald added a subscriber: StephenFan.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h:70-96
+  Expected<ExecutorAddrRange> reserve(size_t NumBytes) {
+    std::promise<Expected<ExecutorAddrRange>> P;
+    auto F = P.get_future();
+    reserve(NumBytes, [&](auto R) { P.set_value(std::move(R)); });
+    return F.get();
+  }
+
----------------
argentite wrote:
> lhames wrote:
> > Synchronous versions of the APIs? Very interesting. I've been assuming that `Mapper` would _only_ have `MappingJITLinkMemoryManager` as a client, and that we'd eventually end up moving the interface into that class (or making it a concept and having `MappingJITLinkMemoryManager` take a template argument), but maybe it's worth having as a utility in its own right.
> > 
> > Did you have a use-case in mind?
> Not really. it just felt nicer when writing the tests. We can remove them.
I think you should sink them into the testcase as free functions for now. We can lift them back into the class later if we decide we want to encourage this kind of direct use of the `Mapper` interface.


================
Comment at: llvm/unittests/ExecutionEngine/Orc/MemoryMapperTest.cpp:31
+
+TEST(ExecutorSharedMemoryManagerTest, AllocFinalizeFree) {
+  int InitializeCounter = 0;
----------------
lhames wrote:
> Could you add some comments to this function describing the steps? E.g.
> ```
> // Define counters -- these will be incremented/decremented in
> // finalize/dealloc actions to verify that the actions are run.
> ```
Just noticed -- this test should be renamed too. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127491