[PATCH] D131930: [mlgo] Add in-development instruction based features for regalloc advisor

[Aiden Grossman via Phabricator via llvm-commits]

aidengrossman added inline comments.


================
Comment at: llvm/lib/CodeGen/MLRegallocEvictAdvisor.cpp:925
 
+void MLEvictAdvisor::extractInstructionFeatures(
+    SmallVectorImpl<std::tuple<SlotIndex, SlotIndex, size_t>> &LRPosInfo)
----------------
mtrofin wrote:
> this deserves more comments about: the whole structure of the data packet you're preparing and the various steps along the way - it'll greatly help with maintainability, etc.
I've added quite a few more comments on the structure of the extraction algorithm itself and the structure of the data that it extracts. Let me know if anything is unclear or too verbose and I'll work on fixing it.


================
Comment at: llvm/lib/CodeGen/MLRegallocEvictAdvisor.cpp:1033
+  if (EnableDevelopmentFeatures) {
+    // Skip FeatureIDs::FeatureCount
+    for (; CurrentFeature < FeatureIDs::FeaturesWithDevelopmentCount - 1;
----------------
mtrofin wrote:
> nit: or, `// continue from the feature index the previous loop left off` ?
> 
This was left over from some experimentation that I have since removed and thus was commenting a line that isn't even there. I've changed it to note your suggestion and also to explain why the indexing is slightly odd (only going to `FeaturesWithDevelopmentCount -1 1` rather than `FeaturesWithDevelopmentCount` itself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131930