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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/lib/CodeGen/MLRegallocEvictAdvisor.cpp:925
 
+void MLEvictAdvisor::extractInstructionFeatures(
+    SmallVectorImpl<std::tuple<SlotIndex, SlotIndex, size_t>> &LRPosInfo)
----------------
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.


================
Comment at: llvm/lib/CodeGen/MLRegallocEvictAdvisor.cpp:926
+void MLEvictAdvisor::extractInstructionFeatures(
+    SmallVectorImpl<std::tuple<SlotIndex, SlotIndex, size_t>> &LRPosInfo)
+    const {
----------------
may be more readable to have a small struct with the 2 slot indices and size_t with nice readable names, and then avoid the whole std::get<x> business.


================
Comment at: llvm/lib/CodeGen/MLRegallocEvictAdvisor.cpp:942
+          LIS->getInstructionFromIndex(CurrentIndex);
+      if (CurrentMachineInstruction == nullptr) {
+        CurrentIndex = CurrentIndex.getNextIndex();
----------------
nit: we generally do if (!CurrentMachineInstruction).


================
Comment at: llvm/lib/CodeGen/MLRegallocEvictAdvisor.cpp:1033
+  if (EnableDevelopmentFeatures) {
+    // Skip FeatureIDs::FeatureCount
+    for (; CurrentFeature < FeatureIDs::FeaturesWithDevelopmentCount - 1;
----------------
nit: or, `// continue from the feature index the previous loop left off` ?



================
Comment at: llvm/test/CodeGen/MLRegalloc/dev-mode-extra-features-logging.ll:52
+; CHECK: key: \"is_free\"
\ No newline at end of file

----------------
add a new line 


================
Comment at: llvm/test/CodeGen/MLRegalloc/dev-mode-extra-features-logging.ll:2
+; REQUIRES: have_tf_api
+; REQUIRES: x86_64-linux
+;
----------------
aidengrossman wrote:
> mtrofin wrote:
> > Because the output very verbose, perhaps just checking specific values may be more maintainable? Like checking that you counted things correctly in your 33x300 matrix - add some comments maybe about what the output looks like and why certain sentinel values are expected. This would also help one identify, later, if the test breaks legitimately due to a change (e.g. some machine instruction is now not present anymore -> of course the features change) vs due to a bug.
> That's a very good point. The test could potentially be pretty fragile if checking the exact code output. I reworked the check to get rid of the exact diff and added a lot more FileCheck checks along with comments to make sure that the output looks as expected to some reasonable degree.
thanks, this looks way more manageable!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131930