[PATCH] D131209: [mlgo] Add ability to create feature-gated development features in regalloc advisor

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/lib/CodeGen/MLRegallocEvictAdvisor.cpp:334
+      : RegAllocEvictionAdvisorAnalysis(AdvisorMode::Release) {
+    InputFeatures = {RA_EVICT_FEATURES_LIST(_DECL_FEATURES)};
+  }
----------------
aidengrossman wrote:
> mtrofin wrote:
> > don't init the static in the ctor. Init it outside (i.e. it can be init-ed where it was before)
> > 
> > also, why make it a member? that seems unrelated change
> I've moved the initialization of `InputFeatures` and `TrainingInputFeatures` to the constructors of the Analyses that use them because adding extra features at runtime requires the modification of these vectors at runtime. I've kept them as static to share a single copy per analysis across the entire invocation of LLVM (although I don't believe it should matter as if I'm understanding everything correctly, there's only one analysis created per invocation). I've moved them to being class members as I believe it makes reading the code more clear (versus sharing commonalities between the advisors and then extending the vectors in the constructors), but I can change it around if desired. This is necessary for the goal of the patch (refactoring to allow for runtime enabled features) if I'm not mistaken and there isn't some approach I'm unaware of, but I can rescope if desired.
Ack on the initialization, but let's not make them static then. The analysis is shared, indeed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131209