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

[Sean Silva via Phabricator via llvm-commits]

silvas added a comment.

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 :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74583