[PATCH] D114831: [NFC][regalloc] Move ExtraRegInfo and related to LiveRangeStageManager

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin marked an inline comment as done.
mtrofin added inline comments.


================
Comment at: llvm/lib/CodeGen/RegAllocEvictionAdvisor.h:102-107
+  MachineRegisterInfo *const MRI;
+  unsigned NextCascade = 1;
+
+public:
+  LiveRangeStageManager(MachineRegisterInfo *MRI) : MRI(MRI) {
+    ExtraRegInfo.resize(MRI->getNumVirtRegs());
----------------
MatzeB wrote:
> mtrofin wrote:
> > MatzeB wrote:
> > > Can we keep the MRI reference out of the interface?
> > The goal was to avoid needing to pass the exact same thing (the MRI->getNumVirtRegs()) at the points where we need to resize. 
> > 
> > So I learn: what's the downside of MRI here?
> > 
> > Would passing the `VirtRegMap` be a reasonable alternative? 
> > So I learn: what's the downside of MRI here?
> 
> No real downside.
> 
> I would just say that reducing dependencies/entanglement with other classes (switching to `VirtRegMap` is as bad in my view). I just think it's a worthwhile general goal when creating new abstractions. And this seemed to be fixable without any real efforts/or lost convenience.
> 
> > The goal was to avoid needing to pass the exact same thing (the MRI->getNumVirtRegs()) at the points where we need to resize.
> 
> It just seemed to me like we can simply get rid of the need for `getNumVirtRegs()` in the `setStage`/`setCascade` functions by switching to the `grow()` interface. I hope you can see the code suggestions I made in phabricator that show that (I think phab sometimes does not reproduce them properly in the E-Mails, so just making sure we're not talking past each other because you don't see the suggestions)
> 
> 
> Anyway these here are nitpicks/suggestions, no harm done if we cannot implement them after all.
I like the .grow() idea, my concern was wether we'd see any compile time impact. Let me try it and see.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114831