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

[Carl Ritson via Phabricator via llvm-commits]

critson added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:158
+
+    if (Cur < Blocks.size())
+      MBB = Blocks[Cur++];
----------------
foad wrote:
> ruiling wrote:
> > 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.
> "for most cases we would not have too much blocks in an if-else structure": this makes me a bit nervous. People write big shaders, so some people probably write big shaders with an if-then-else wrapped around the whole thing.
OK, I understand what the loop does now.  I clearly understood it first time, but did get confused on the second reading.
As Jay raises the is_contained in the loop may be executed a lot with a big if-else in a shader?
Maybe this should be a SmallSetVector?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102212