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

Thu Apr 27 12:55:33 PDT 2023

jacobhegna 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 {                                                                  \
----------------
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 

================
Comment at: llvm/lib/Analysis/MLInlinerEmitCRunner.h:28
+#define REGISTER_BUFFER(cpp_name, py_name, _)                                  \
+  do {                                                                         \
+    setUpBufferForTensor(BufferIdx, Inputs[BufferIdx],                         \
----------------
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

================
Comment at: llvm/lib/Analysis/models/emitc/InlineOzTestModel.emitc.h:17
+// been modified to fit the needs of generated C++ models in LLVM.
+#include <memory>
+#include <string>
----------------
mtrofin wrote:
> can we add a "this is generated, avoid modifying me by hand" type of thing loud somewhere in the comment? same for the .cpp
fixed

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146483