[PATCH] D156491: [RA] Split a virtual register in cold blocks if it is not assigned preferred physical register

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet accepted this revision.
qcolombet added a comment.
This revision is now accepted and ready to land.

Thanks for the clarifications @Carrot



================
Comment at: llvm/lib/CodeGen/RegAllocGreedy.cpp:2404
+  if (!NewVRegs.empty())
+    return 0;
 
----------------
Carrot wrote:
> qcolombet wrote:
> > Carrot wrote:
> > > qcolombet wrote:
> > > > qcolombet wrote:
> > > > > Splitting decisions are supposed to be taken later (`trySplit`), this breaks the general algorithm's flow.
> > > > Concretely, I'm asking if you could do this kind of splitting in the splitting heuristic.
> > > > You may want to add a new stage in the algorithm (and that'll come at a compile time cost.)
> > > The problem is we need to decide between a physical register and split, if we move it to trySplit, we will miss the possibility of assigning a physical register to the whole virtual register. trySplit compares splits against different physical registers, we need to compare split against non-split.
> > > Function tryAssignCSRFirstTime does similar thing. trySplitAroundHintReg is even more special because it tries to split against only 1 physical register.
> > > 
> > > Function tryAssignCSRFirstTime does similar thing.
> > 
> > True but I feel it is justified because these physreg are not free to use the first time.
> > I don't see this argument applying here.
> > 
> > > if we move it to trySplit, we will miss the possibility of assigning a physical register to the whole virtual register.
> > 
> > Hmm, I don't get that part, we're going to split in `trySplitAroundHintReg`, how do we get to assign the same physreg to the whole vreg?
> > 
> > >  we need to compare split against non-split.
> > 
> > I don't get that part either.
> > For a given vreg, the first time we get through trySplit, we compare non-split against split.
> > 
> > I'm not totally opposed to the patch, but I still fail to see why it doesn't fit in the regular splitting mechanism and hence why we need a special mechanism for this.
> > > Function tryAssignCSRFirstTime does similar thing.
> > 
> > True but I feel it is justified because these physreg are not free to use the first time.
> > I don't see this argument applying here.
> > 
> The key point here is we need to compare the cost of using CSR(for the first time) against the cost of splitting the virtual register. Then we can decide to split the virtual register or assign a single physical register to it.
> 
> > > if we move it to trySplit, we will miss the possibility of assigning a physical register to the whole virtual register.
> > 
> > Hmm, I don't get that part, we're going to split in `trySplitAroundHintReg`, how do we get to assign the same physreg to the whole vreg?
> > 
> We already get a physical register before calling trySplitAroundHintReg. If trySplitAroundHintReg can't find a split better than a non-hint physical register, it returns false, and the caller tryAssign can simply return the physical register for the whole vreg.
> 
> In function trySplitAroundHintReg it first computes the cost of using non-hint physical register for the whole vreg, then it passes the cost to calculateRegionSplitCostAroundReg, calculateRegionSplitCostAroundReg only records split candidate with lower cost, so trySplitAroundHintReg actually compares using a non-hint physical register for the whole vreg against splitting the vreg around the hint register.
> 
> > >  we need to compare split against non-split.
> > 
> > I don't get that part either.
> > For a given vreg, the first time we get through trySplit, we compare non-split against split.
> >
> Sorry for the confusion of non-split. I mean compare split against using a single physical register.
> 
> > I'm not totally opposed to the patch, but I still fail to see why it doesn't fit in the regular splitting mechanism and hence why we need a special mechanism for this.
> 
> 
Got it. Makes sense.

Now, I understand the comment around `trySplitAroundHintReg`.


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

https://reviews.llvm.org/D156491