[PATCH] D122200: [AMDGPU] Split waterfall loop exec manipulation

[Ruiling, Song via Phabricator via llvm-commits]

ruiling added a comment.

I agree this is a good idea and the change looks good, just only a few minor comments. btw, Is this the only case that we modify EXEC in middle of a block?



================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:337-338
+  // Collect loop instructions, potentially spanning multiple blocks
+  for (auto MBBI = LoopHeader->getIterator(),
+            MBBE = std::next(LoopEnd->getIterator());
+       MBBI != MBBE; ++MBBI) {
----------------
critson wrote:
> arsenm wrote:
> > I've thought about adding some kind of iterator which would automatically cross fallthrough blocks
> I can certainly see it being useful in a few places.
I would suggest you simply push LoopHeader and LoopEnd into the Blocks here, otherwise we have to go through the blocks by checking against predecessors/successors. The block appears between two blocks in the block-list may not appear between them in the CFG.
You can also place an assert the LoopHeader has unique successor, which should be LoopEnd.


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeVGPRLiveRange.cpp:570
   // after the loop. So we know that the old reg is not live throughout the
   // whole block anymore.
+  for (auto *Block : Blocks)
----------------
Please help update the comment a little bit to reflect the new code. It is better to place an assert that Blocks.size() <=2 here. The reason is if we have more blocks, we can not clear the AliveBlocks bit in this simple way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122200