[PATCH] D127281: [Greedy RegAlloc] Fix the handling of split register in last chance re-coloring.

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added inline comments.


================
Comment at: llvm/lib/CodeGen/RegAllocGreedy.cpp:1907
       std::tie(LI, PhysReg) = RecolorStack[I];
-      Matrix->assign(*LI, PhysReg);
+      if (!LI->empty() && !MRI->reg_nodbg_empty(LI->reg()))
+        Matrix->assign(*LI, PhysReg);
----------------
arsenm wrote:
> The possibility for splitting during last chance recoloring confused me when I was last looking at this here. Is it a problem that the split was also not rolled back as well?
Exactly, the problems comes from the fact that register we are trying to recolor is split in splirOrSelect and split always results in no assignment to physical register, as a result register is not recolored and we should do a roll back.

Current rollback implementation just returns back register we wanted to recolor to original physical register however this register is split and has no uses now.

In an example:
We have register A to try last chance to color.
We take the physical register P and find all registers assigned to P and interfere with live interval of A. Let it be only one register R.
Then we unassign R from P and assign P to A. Fix the register P (do not use it in allocation) and try to recolor R.
As a result of selectOrSplit R is split into R1, R2, R3. R has no uses in machine instructions now, its uses are replaced with R1, R2, R3 and corresponding copy instructions are inserted to connect R1, R2, R3.
selectOrSplit always returns no assignment and R1, R2, R3 as new registers to be added again into queue and process them further on regular basis.
As soon as we are in last chance color, we get no assignment of R to physical register and states this as a failure of recoloring.
So we do a rollback. In this rollback we just return back an assignment of R to P. But R has no uses now and it leads to different assertions later when handling such registers (without uses but with assignment to physical registers).

This is a description of the problem.

Now how to solve this in rollback:
1) This patch just avoid assignment of P to R due to R has no uses. R1, R2, R3 will go to the queue finally and will processed on next iterations (it is actually change you did recently).
2) We can try to assign R1, R2, R3 to P. It has more complexity in implementation due to no easy way to detect that R is actually split into R1, R2, R3. So we need some preparation work.
3) We can reject split in selectOrSplit while we are doing last chance recoloring.
4) We can do even smarter: If we detect split R to R1, R2, R3 we can try to continue recolor R1, R2, R3 or only those Ri which still interfere with A.

May be there are other solutions. But just for bug fix I choose the less intrusive way. I hope it helps and not confuse :)





================
Comment at: llvm/test/CodeGen/AArch64/regalloc-last-chance-recolor-with-split.mir:1
-# RUN: not --crash llc -mattr="+reserve-x28" --start-before=greedy -verify-machineinstrs %s -o - 2>&1 | FileCheck %s
+# RUN: llc -mattr="+reserve-x28" --start-before=greedy -verify-machineinstrs %s -o - 2>&1 | FileCheck %s
 
----------------
arsenm wrote:
> Why still redirect stderr?
Yeah! No need for this more.


================
Comment at: llvm/test/CodeGen/AArch64/regalloc-last-chance-recolor-with-split.mir:3
 
-# CHECK: Non-empty but used interval
+# CHECK-LABEL: ham
 
----------------
arsenm wrote:
> This should get some actual checks, probably should just use update_mir_test_checks
ok, I'll do it but the main idea here was that this does not crash anymore.


================
Comment at: llvm/test/CodeGen/AMDGPU/remaining-virtual-register-operands.ll:8-11
+; The machine verifier complains about usage of register
+; which is marked as killed in previous instruction.
+; This happens due to when register allocator is out of registers
+; it takes the first avialable register.
----------------
arsenm wrote:
> This is the problem D122616 is trying to solve
Thanks for pointing me to.


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

https://reviews.llvm.org/D127281