[PATCH] D102212: [AMDGPU] Add Optimize VGPR LiveRange Pass.

[Ruiling, Song via Phabricator via llvm-commits]

ruiling added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:158
+
+    if (Cur < Blocks.size())
+      MBB = Blocks[Cur++];
----------------
foad wrote:
> critson wrote:
> > This doesn't seem right.
> > You only collect an arbitrary number of blocks?
> > Should ElseBlocks not simply be growing to hold all relevant blocks?
> Cur tells you the first index in Blocks of a block that you have *not* already visited to scan its predecessors. Perhaps it would be clearer as:
> ```
> SetVector<MachineBasicBlock *> Blocks(Endif);
> for (unsigned Cur = 0; Cur < Blocks.size(); ++Cur) {
>   auto *MBB = Blocks[Cur];
>   for (auto *Pred : MBB->predecessors()) {
>     if (Pred != Flow)
>       Blocks.insert(Pred);
>   }
> }
> ```
Hi @critson, does @foad's comment help answer your concern? I don't quite understand your confusion. The idea here is to iteratively visit the predecessors starting from `Endif` and push them into the Blocks and stop if the predecessor is `Flow`. I admit the code here may not quite easy to understand, I struggled for a while when writing this piece of code. @foad's suggestion missed one subtle requirement: the `Endif` should not appear in the final result. and I don't want to remove the first element from the vector after that. I think that was the main reason I wrote it like this. Considering that for most cases we would not have too much blocks in an if-else structure(may be I am wrong?), I think a "find in vector" should not be a big issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102212