[PATCH] D74583: Fix Block::eraseArgument when block arg is also a successor operand.

[River Riddle via Phabricator via llvm-commits]

rriddle accepted this revision.
rriddle added a comment.
This revision is now accepted and ready to land.

In D74583#1876727 <https://reviews.llvm.org/D74583#1876727>, @silvas wrote:

> I couldn't think of a great way to test this. The one place I can think of that could potentially benefit from this uses updatePredTerms=false because it erases successor operands manually for other reasons:
>  https://github.com/llvm/llvm-project/blob/e8358455a2b662bec59bc3971c18346800d7cb00/mlir/lib/Transforms/Utils/RegionUtils.cpp#L278
>
> Maybe the current behavior is a "feature" and not a bug? That is, this function has a contract (currently implicit) saying that the BlockArgument being erased must have no uses? Thinking about it more, doing dropAllUses() is a more general solution and changes the contract of eraseArgument.
>
> So I think it boils down to the contract of eraseArgument. Do we want:
>
> 1. Precondition: the BlockArgument has no uses (current behavior at HEAD)
> 2. Precondition: the BlockArgument has no uses besides successor operands of a predecessor (behavior in this patch)
> 3. Precondition: the BlockArgument can have uses, and we will dropAllUses() as part of this call.
>
>   When put that way, I think this current patch is a weird middle-ground.
>
>   Would like to hear Mehdi and River's feedback on this question. I'll also update the eraseArgument comment with the result of the decision :)


I think we can do either 1 or 2. When I originally added the ability to update the predecessor terminators, it was to remove a bit of redundant work that the user would generally end up doing anyways. I'm generally of the opinion that we can treat uses related to block arguments specially, because we can completely reason about their meaning/context/semantics. This is also why 3 feels a bit naughty to me, it feels like it's too easy for the user to hurt themselves that way. One could say that the same logic applies to case 2, but I see it slightly differently as we can do that transformation automatically and know it is correct(i.e. the argument is just a pass through).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74583