Detaching multiple regions from an operation breaks that op

[watermelonwolverine]

op.detach_region(op.non_optional_region)
op.detach_region(op.optional_region1)
op.detach_region(op.optional_region2)

raises an exception:

Traceback (most recent call last):
  ...
  File "/workspace/.venv/lib/python3.10/site-packages/xdsl/irdl/operations.py", line 1823, in fun
    return get_operand_result_or_region(
  File "/workspace/.venv/lib/python3.10/site-packages/xdsl/irdl/operations.py", line 1455, in get_operand_result_or_region
    return args[begin_arg]
IndexError: tuple index out of range

Interesting thing to note is that the exception is not raised while removing it, but seemingly while accessing op.optional_region2.
It seems to me that some internal data structures are not updated during detach_region (?).

[watermelonwolverine]

Something like this works:

optional_region1 = op.optional_region1
optional_region2 = op.optional_region2

op.detach_region(op.non_optional_region)
op.detach_region(optional_region1)
op.detach_region(optional_region2)

[superlopuh]

Indeed, it looks like the IRDL wrapper should take this into account and update the regionSegmentSizes

[superlopuh]

CC @math-fehr

[math-fehr]

Hmm, I'm actually not sure how to deal with that one, because that's fundamental on how operations are actually defined.

That's actually not specific to regions btw, you will also see this for operands or results.
If you have non-optional regions, you will actually get the same problem.

So the problem is, how do you get op.optional_region2?
You take the regions at index [1 + regionSegmentSizes[0], 1 + regionSegmentSizes[0] + regionSegmentSizes[1].
This is because accessors to operations expect the operation to verify.
Here, because you detached the first region, everything breaks, because the operation doesn't verify anymore, because
you detached a region that was expected to always be there. So all accessors are completely wrong.

I somehow feel we have the wrong API to detach regions, because as soon as we detach regions, we break
the operation accessors.

[watermelonwolverine]

So, if I understood correctly, there are two states an operation can be in: A "verified" state in which it is after verification in which it should work and a "modified" state, where no guarantees can be made?

Maybe we can embrace this a bit by removing the "detach" functions and adding a "dissolve" function instead that dissolves an operation into its constituent parts which can then be re-used.

This would be useful during rewriting where you don't need the old op anymore, but you also don't want to clone everything to keep performance up.

Alternatively we could replace a detached region with a "poison" object that keeps the number of regions the same but causes verification to fail. This would allow detaching several individual things from an operation.

[math-fehr]

Yes that's kind of it, whenever you start modifying an operation, you might enter a place where the operation doesn't verify anymore.

The difficulty though, is that sometimes you can detach a region without it being incorrect. For instance, if you have a variadic number
of regions, then removing a region may be valid (and something you want to do).

So yeah, maybe dissolve is something we need, but also it wouldn't allow us to mention regions/operands/results by name, which is annoying.

I think one thing we should maybe do is prefer using functions that move region contents, rather than removing the region (and removing the region only when necessary).