[PATCH] D112731: [AMDGPU] Really preserve LiveVariables in SILowerControlFlow

[Ruiling, Song via Phabricator via llvm-commits]

ruiling added inline comments.


================
Comment at: llvm/lib/CodeGen/LiveVariables.cpp:672
 
+void LiveVariables::recomputeLivenessForSingleDef(Register Reg) {
+  assert(Reg.isVirtual());
----------------
`liveness` sounds unnecessary to me. It is better to explicitly say this is for virtual register to align with other public functions of LiveVariables. Something like recomputeForSingleDefVirtReg()?


================
Comment at: llvm/lib/CodeGen/LiveVariables.cpp:695-699
+    if (UseMI.isPHI()) {
+      // If Reg is used in a phi then it is live-out of the corresponding
+      // predecessor.
+      unsigned Idx = UseMI.getOperandNo(&UseMO);
+      LiveOutBlocks.push_back(UseMI.getOperand(Idx + 1).getMBB());
----------------
This is not quite accurate. if the user is a PHI. we need to handle differently if the user appears in the same block as the define block.

For the pattern drawn at line 148 (incoming block is the same as define block), we don't treat the Reg is alive through the incoming block, it is only alive from the definition to the end. If the incoming block is different from define block, we need to mark the Reg is alive through the incoming block.


================
Comment at: llvm/lib/CodeGen/LiveVariables.cpp:731
+      if (MI.readsRegister(Reg)) {
+        if (!MI.killsRegister(Reg)) {
+          MI.addRegisterKilled(Reg, nullptr);
----------------
foad wrote:
> arsenm wrote:
> > Won't this do the right thing if you unconditionally call addRegisterKilled?
> It could give you duplicate entries in the Kills vector which seems undesirable.
I think you don't need to require that the isKill flag was correctly setup before calling this function, finding out the new kill after modification is a non-trivial work for general case. It is really handy to find the kill instructions here. Doing a reverse iteration over the instructions will just give you the kill instruction, that is the first instruction we meet.
You can just remove the `killsRegister()` check here and also stop pushing into Kills when iterating over the uses earlier in this function .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112731