[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:1001
+            LRPosInfo[OverlapCheckCurrentSegment].Pos;
+        Runner->getTensor<int64_t>(FeatureIDs::instructions_mapping -
+                                   1)[OverlapCurrentSegmentPosition *
----------------
aidengrossman wrote:
> mtrofin wrote:
> > can't `LRPosInfo[OverlapCheckCurrentSegment].End < CurrentIndex`? So we know the CurrentIndex is below Begin, but it could also be below this segment's end?
> > 
> > Current instruction: 5
> > CurrentSegment: [1,7)
> > NextSegment: [2,4)
> > NextNextSegment: [2, 8)
> > 
> `LRPosInfo[OverlapCheckCurrentSegment].End` can be less than `CurrentIndex`, but it shouldn't be missing any instructions. It's not checking if `CurrentIndex` is below begin, but rather if `CurrentIndex` is above the beginning of the current segment, because if the beginning of the current segment being checked is greater than the current index under analysis, it is guaranteed that the segment currently being processed as well as all future segments don't contain the currently being processed index due to the segments being sorted in ascending order by beginning index.
Nevermind my previous comment. Not sure what I was thinking there. This was an actual issue and I ended up catching it while looking through some unit test cases. Should be fixed on the next push as I added in a condition to only set the relevant element of the `instructions_mapping` matrix if `CurrentIndex <= LRPosInfo[OverlapCheckCurrentSegment].End`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131930