[PATCH] D131015: [LV] Track all IR blocks corresponding to VPBasicBlock

[Philip Reames via Phabricator via llvm-commits]

reames abandoned this revision.
reames added a comment.

In D131015#3772523 <https://reviews.llvm.org/D131015#3772523>, @fhahn wrote:

>> The existing code is only correct when a VPBasicBlock in the header or latch position corresponds to exactly one BasicBlock. This happens to be true as the only VPBasicBlock which corresponds to more than one BasicBlock today is a VPReplicateRecipe which (as an implementation detail) can't be either a loop header or latch.
>
> I am not sure this is accurate, at the moment a non-predicated VPReplicateRecipe can be in any block I think.  Predicated VPReplicateRecipes must be in a VPBasicBlock in a VPRegionBlock. IIUC this may become an issue after D131118 <https://reviews.llvm.org/D131118> if regular VPReplicateRecipes could be expanded to multiple basic blocks?
>
> If that's the issue, it would probably be clearer if this is modeled explicitly, by putting such recipes into their own region, more faithfully representing the fact that a loop will be generated for them.

The original comment was correct.  The code as written is incorrect if a VPReplicateRecipe which corresponds to two or more BasicBlocks if that recipe were either header or exiting block of the loop.

However, as my comment said, this is impossible in the current code.  A predicated replicate recipe can't be either of those positions (since it's contained by the VPRegionBlock), and a non-predicated one always corresponds to one basic block.

Your suggestion is to essentially add an invariant that a replicate region only contains more than one BB if it's contained in a VPRegionBlock.  I am not opposed to that, but I'm also not motivated to make that change.  It's significantly more invasive, and I just don't care that much.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131015