[PATCH] D146483: Add infrastructure to use EmitC-generated models for inlining.

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/include/llvm/Analysis/EmitCModelRegistry.h:109
+// Macro which simplifies registering models with the registry.
+#define REGISTER_EMITC_MODEL(ModelRunnerType, EmitCModelType)                  \
+  namespace {                                                                  \
----------------
jacobhegna wrote:
> mtrofin wrote:
> > could this macro also #include the header - I think it'd be totally reasonable to follow a canonical naming convention where the header name is trivially derivable from `EmitCModelType`, for example.
> mmm we can if you want, but the problem is that I think we will eventually store models for different passes in different places - for example, the regalloc models might end up somewhere in llvm/lib/CodeGen. we can put the include here if you want, but not sure it's the most useful optimization 
maybe add a parameter that is the header location, so this becomes 1 gesture rather than 2 (easier to discover what needs to be given, too)


================
Comment at: llvm/lib/Analysis/MLInlinerEmitCRunner.h:28
+#define REGISTER_BUFFER(cpp_name, py_name, _)                                  \
+  do {                                                                         \
+    setUpBufferForTensor(BufferIdx, Inputs[BufferIdx],                         \
----------------
jacobhegna wrote:
> mtrofin wrote:
> > would it be possible for the emitC-ed code to just give us a `void* lookupTensorBuffer(const std::string &)` - it'd also keep the codegen-ed files smaller
> fixed
ok but now you can avoid the macro-ing, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146483