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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added a comment.

Thanks for doing this!

High level, the dependency from model to LLVM should go away. Also `emitc` stuff should be implementation detail (i.e. not in any headers)

The models can then have their `CMakeLists.txt`, which IIUC it's the expected way for subdirectories.



================
Comment at: llvm/include/llvm/Analysis/EmitCModelRegistry.h:55
+
+  std::unordered_map<std::string, std::unique_ptr<ModelT>> Models;
+};
----------------
`StringMap`?


================
Comment at: llvm/include/llvm/Analysis/EmitCModelRegistry.h:60
+// an object of this type is all you need to do to register the model.
+template <class ModelT> class EmitCModelRegistrationHandle {
+public:
----------------
why do we need this, wouldn't the macro below be sufficient?


================
Comment at: llvm/include/llvm/Analysis/EmitCTensor.h:1
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
----------------
won't this be part of the model package, i.e. something that's (maybe cloned) per-model?


================
Comment at: llvm/include/llvm/Analysis/MLInlineEmitCModel.h:1
+//===- MLInlineEmitCModel.h -- Model for inlining EmitC Models --*- C++ -*-===//
+//
----------------
Nit: isn't it "Model for inlining Oz"?


================
Comment at: llvm/include/llvm/Analysis/MLInlineEmitCModel.h:35
+  // not strictly necessary.
+  virtual void set_inlining_default(emitc::Tensor<int64_t, 1> x) {}
+  virtual void set_step_type(emitc::Tensor<int32_t, 1> x) {}
----------------
can't these not be surfaced to begin with?


================
Comment at: llvm/lib/Analysis/MLInlinerEmitCRunner.h:26
+template <class T, int64_t... Dims>
+emitc::Tensor<T, Dims...> convertBufferToEmitCTensor(void *Buffer,
+                                                     TensorSpec Spec) {
----------------
This should be in the emit-ed code, so `emitc` stuff shouldn't appear in any llvm-facing APIs.


================
Comment at: llvm/lib/Analysis/MLInlinerEmitCRunner.h:63
+    size_t idx = static_cast<size_t>(FeatureIndex::cpp_name);                  \
+    Model->set_##py_name(convertBufferToEmitCTensor<int64_t, 1>(               \
+        getTensorUntyped(idx), InputSpecs[idx]));                              \
----------------
can't we set the buffers in the ctor once? they don't get reallocated.


================
Comment at: llvm/lib/Analysis/models/emitc/InlineOzTestModel.emitc.h:20
+namespace llvm::emitc::generated {
+class InlineOzTestModel : public ::llvm::MLInlineOzEmitCModel {
+private:
----------------
Not sure what the inheritance relationship buys us. We already have a common mechanism for referring to features by their enum-named index. This also seems to add a tight coupling between codegen and llvm.

If all we need is a way to identify groups of models in the registry, that could be done with the address of a static specific to each consumer, i.e. MLInlineAdvisor could have a static char ID = '\0' and the address of that ID is the key (like the legacy PM does things, too) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146483