[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 {                                                                  \
----------------
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.


================
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:
----------------
jacobhegna wrote:
> mtrofin wrote:
> > why do we need this, wouldn't the macro below be sufficient?
> we need to call `EmitCModelRegistry<ModelT>::get().addModel(...)` before `main()` runs in the final linked binary. the easiest way to do that is to define a global object whose constructor runs that code - that's the  point of the `EmitCModelRegistrationHandle`. We can't (as far as I know) just have the macro because then we wouldn't have a "context" to run the needed code (right now, that context is the constructor of `...Handle`.
> 
> this is basically how `cl::opt` works, too.
> 
> Note that this mechanism is about to change when I update the patch. Instead of registering instantiated `unique_ptr`s of each model, I register a factory method which is capable of creating a `MLModelRunner` pointer. The reason for the change is because now that the emitc models don't share a common base class, there isn't a good way to store them all in a common registry (but we can store the `MLModelRunner*` because they use a subclass templated on the emitc model.
ah, makes sense. thanks!


================
Comment at: llvm/include/llvm/Analysis/InlineModelFeatureMaps.h:25
 #define INLINE_COST_FEATURE_ITERATOR(M)                                        \
-  M(SROASavings, "sroa_savings")                                               \
-  M(SROALosses, "sroa_losses")                                                 \
-  M(LoadElimination, "load_elimination")                                       \
-  M(CallPenalty, "call_penalty")                                               \
-  M(CallArgumentSetup, "call_argument_setup")                                  \
-  M(LoadRelativeIntrinsic, "load_relative_intrinsic")                          \
-  M(LoweredCallArgSetup, "lowered_call_arg_setup")                             \
-  M(IndirectCallPenalty, "indirect_call_penalty")                              \
-  M(JumpTablePenalty, "jump_table_penalty")                                    \
-  M(CaseClusterPenalty, "case_cluster_penalty")                                \
-  M(SwitchPenalty, "switch_penalty")                                           \
-  M(UnsimplifiedCommonInstructions, "unsimplified_common_instructions")        \
-  M(NumLoops, "num_loops")                                                     \
-  M(DeadBlocks, "dead_blocks")                                                 \
-  M(SimplifiedInstructions, "simplified_instructions")                         \
-  M(ConstantArgs, "constant_args")                                             \
-  M(ConstantOffsetPtrArgs, "constant_offset_ptr_args")                         \
-  M(CallSiteCost, "callsite_cost")                                             \
-  M(ColdCcPenalty, "cold_cc_penalty")                                          \
-  M(LastCallToStaticBonus, "last_call_to_static_bonus")                        \
-  M(IsMultipleBlocks, "is_multiple_blocks")                                    \
-  M(NestedInlines, "nested_inlines")                                           \
-  M(NestedInlineCostEstimate, "nested_inline_cost_estimate")                   \
-  M(Threshold, "threshold")
+  M(SROASavings, sroa_savings, "")                                             \
+  M(SROALosses, sroa_losses, "")                                               \
----------------
pls add a small doc accordingly


================
Comment at: llvm/include/llvm/Analysis/InlineModelFeatureMaps.h:84
 #define INLINE_FEATURE_ITERATOR(M)                                             \
-  M(CalleeBasicBlockCount, "callee_basic_block_count",                         \
+  M(CalleeBasicBlockCount, callee_basic_block_count,                           \
     "number of basic blocks of the callee")                                    \
----------------
can you peel this bit of the change separately and just submit it (so the part that adds the self-doc-ing and makes the 2 lists the same)


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


================
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>
----------------
can we add a "this is generated, avoid modifying me by hand" type of thing loud somewhere in the comment? same for the .cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146483