[PATCH] D104799: [Orc] Add verylazy example for C-bindings

[Valentin Churavy via Phabricator via llvm-commits]

vchuravy marked 2 inline comments as done.
vchuravy added a comment.

> Regarding the MaterializationResponsibility API: it would be good to add the addDependencies and addDependenciesForAll methods. These declare dependencies for each materializing symbol so that ORC can track them in its dependence graph, which in turn ensures that lookups remain safe regardless of what threads the symbols are being materialized on. These methods are an essential part of the MaterializationResponsibility API.

I added methods for both, but I haven't added a test yet. I marked in the unit tests which API methods for MR are not yet covered.

I think we also talked about the type hierarchy for the Layers and that in theory we should allow for some polymorphism instead of requiring specific types.



================
Comment at: llvm/examples/OrcV2Examples/OrcV2CBindingsVeryLazy/OrcV2CBindingsVeryLazy.c:148
+  } else {
+    // Question at lhames: Better way to get LLVMOrcLLJITRef J;
+    LLVMOrcLLJITRef J = (LLVMOrcLLJITRef)Ctx;
----------------
@lhames am I right that I need to pass the LLJIT as the Context?


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/LLJIT.h:188
 
+  Error applyDataLayout(Module &M);
+
----------------
@lhames are you okay with making this public?

We need either that or a way to get the TM from LLJIT. 
For Julia I copied the implementation https://github.com/maleadt/LLVM.jl/blob/600f5cd22252d12c64ff3e00f2921ba101323a70/deps/LLVMExtra/lib/orc.cpp#L229-L244


================
Comment at: llvm/unittests/ExecutionEngine/Orc/OrcCAPITest.cpp:380
+  ASSERT_TRUE(!!Symbols);
+  ASSERT_EQ(NumSymbols, 1);
+
----------------



================
Comment at: llvm/unittests/ExecutionEngine/Orc/OrcCAPITest.cpp:400
+      LLVMJITSymbolGenericFlagsExported | LLVMJITSymbolGenericFlagsCallable, 0};
+  // TODO(lhames & vchuravy) flags returned here and not correct
+  // ASSERT_EQ(TargetSym.Flags.GenericFlags, Flags.GenericFlags);
----------------
@lhames the flags returned here are wrong so I might have a mistake somewhere in the conversion code.


================
Comment at: llvm/unittests/ExecutionEngine/Orc/OrcCAPITest.cpp:409
+
+  // TODO (lhames) test
+  // - LLVMOrcMaterializationResponsibilityDefineMaterializing(
----------------
@lhames Ideas for tests are appreciated :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104799