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

Fri Oct 29 05:45:59 PDT 2021

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

================
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());
----------------
ruiling wrote:
> foad wrote:
> > ruiling wrote:
> > > 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.
> > I think my code is correct. I am only saying it is live-**out** of the corresponding predecessor, not live-through. The "Iterate ..." loop below handles the case where Reg is live-out of DefBB.
> I see the problem, your interpretation of `live-out` does not match what it means in LLVM. You can take a look at `LiveVariables::isLiveOut()`. We are not saying that a register is live-out of a block if it is only used by a PHI in successor block. I would suggest renaming `LiveOutBlocks` to `LiveToEndBlocks`. So we don't have conflicting interpretations of `live-out`. Please also fix the use of `live-out` in comments.
Thanks for explaining. I fixed it, but I still think my terminology is better :)

The way I think of it is that phi uses occur on an edge between two blocks. A phi use still means that the value is live-out of the predecessor, since it happens after the end of the predecessor (and before the start of the successor).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112731