This commit fixes a crash in the dialect conversion when applying a
signature conversion to a block inside of a detached region.
This fixes an issue reported in
4114d5be87 (r1691809730).
This commit simplifies the handling of dropped arguments and updates
some dialect conversion documentation that is outdated.
When converting a block signature, a `BlockTypeConversionRewrite` object
and potentially multiple `ReplaceBlockArgRewrite` are created. During
the "commit" phase, uses of the old block arguments are replaced with
the new block arguments, but the old implementation was written in an
inconsistent way: some block arguments were replaced in
`BlockTypeConversionRewrite::commit` and some were replaced in
`ReplaceBlockArgRewrite::commit`. The new
`BlockTypeConversionRewrite::commit` implementation is much simpler and
no longer modifies any IR; that is done only in `ReplaceBlockArgRewrite`
now. The `ConvertedArgInfo` data structure is no longer needed.
To that end, materializations of dropped arguments are now built in
`applySignatureConversion` instead of `materializeLiveConversions`; the
latter function no longer has to deal with dropped arguments.
Other minor improvements:
- Add more comments to `applySignatureConversion`.
Note: Error messages around failed materializations for dropped basic
block arguments changed slightly. That is because those materializations
are now built in `legalizeUnresolvedMaterialization` instead of
`legalizeConvertedArgumentTypes`.
This commit is in preparation of decoupling argument/source/target
materializations from the dialect conversion.
This is a re-upload of #96207.
This commit fixes a bug in the dialect conversion. During a 1:N
signature conversion, the dialect conversion did not insert a cast back
to the original block argument type, producing invalid IR.
See `test-block-legalization.mlir`: Without this commit, the operand
type of the op changes because an `unrealized_conversion_cast` is
missing:
```
"test.consumer_of_complex"(%v) : (!llvm.struct<(f64, f64)>) -> ()
```
To implement this fix, it was necessary to change the meaning of
argument materializations. An argument materialization now maps from the
new block argument types to the original block argument type. (It now
behaves almost like a source materialization.) This also addresses a
`FIXME` in the code base:
```
// FIXME: The current argument materialization hook expects the original
// output type, even though it doesn't use that as the actual output type
// of the generated IR. The output type is just used as an indicator of
// the type of materialization to do. This behavior is really awkward in
// that it diverges from the behavior of the other hooks, and can be
// easily misunderstood. We should clean up the argument hooks to better
// represent the desired invariants we actually care about.
```
It is no longer necessary to distinguish between the "output type" and
the "original output type".
Most type converter are already written according to the new API. (Most
implementations use the same conversion functions as for source
materializations.) One exception is the MemRef-to-LLVM type converter,
which materialized an `!llvm.struct` based on the elements of a memref
descriptor. It still does that, but casts the `!llvm.struct` back to the
original memref type. The dialect conversion inserts a target
materialization (to `!llvm.struct`) which cancels out with the other
cast.
This commit also fixes a bug in `computeNecessaryMaterializations`. The
implementation did not account for the possibility that a value was
replaced multiple times. E.g., replace `a` by `b`, then `b` by `c`.
This commit also adds a transform dialect op to populate SCF-to-CF
patterns. This transform op was needed to write a test case. The bug
described here appears only during a complex interplay of 1:N signature
conversions and op replacements. (I was not able to trigger it with ops
and patterns from the `test` dialect without duplicating the `scf.if`
pattern.)
Note for LLVM integration: Make sure that all
`addArgument/Source/TargetMaterialization` functions produce an SSA of
the specified type.
Depends on #98743.
This commit moves the argument materialization logic from
`legalizeConvertedArgumentTypes` to
`legalizeUnresolvedMaterializations`.
Before this change:
- Argument materializations were created in
`legalizeConvertedArgumentTypes` (which used to call
`materializeLiveConversions`).
After this change:
- `legalizeConvertedArgumentTypes` creates a "placeholder"
`unrealized_conversion_cast`.
- The placeholder `unrealized_conversion_cast` is replaced with an
argument materialization (using the type converter) in
`legalizeUnresolvedMaterializations`.
- All argument and target materializations now take place in the same
location (`legalizeUnresolvedMaterializations`).
This commit brings us closer towards creating all source/target/argument
materializations in one central step, which can then be made optional
(and delegated to the user) in the future. (There is one more source
materialization step that has not been moved yet.)
This commit also consolidates all `build*UnresolvedMaterialization`
functions into a single `buildUnresolvedMaterialization` function.
This commit simplifies the handling of dropped arguments and updates
some dialect conversion documentation that is outdated.
When converting a block signature, a `BlockTypeConversionRewrite` object
and potentially multiple `ReplaceBlockArgRewrite` are created. During
the "commit" phase, uses of the old block arguments are replaced with
the new block arguments, but the old implementation was written in an
inconsistent way: some block arguments were replaced in
`BlockTypeConversionRewrite::commit` and some were replaced in
`ReplaceBlockArgRewrite::commit`. The new
`BlockTypeConversionRewrite::commit` implementation is much simpler and
no longer modifies any IR; that is done only in `ReplaceBlockArgRewrite`
now. The `ConvertedArgInfo` data structure is no longer needed.
To that end, materializations of dropped arguments are now built in
`applySignatureConversion` instead of `materializeLiveConversions`; the
latter function no longer has to deal with dropped arguments.
Other minor improvements:
- Improve variable name: `origOutputType` -> `origArgType`. Add an
assertion to check that this field is only used for argument
materializations.
- Add more comments to `applySignatureConversion`.
Note: Error messages around failed materializations for dropped basic
block arguments changed slightly. That is because those materializations
are now built in `legalizeUnresolvedMaterialization` instead of
`legalizeConvertedArgumentTypes`.
This commit is in preparation of decoupling argument/source/target
materializations from the dialect conversion.
This commit removes a `FIXME` in the code base that was in place because
of patterns that used the dialect conversion API incorrectly. Those
patterns have been fixed and the workaround is no longer needed.
Remove a TODO in the dialect conversion code base when materializing
unresolved conversions:
```
// FIXME: Determine a suitable insertion location when there are multiple
// inputs.
```
The implementation used to select an insertion point as follows:
- If the cast has exactly one operand: right after the definition of the
SSA value.
- Otherwise: right before the cast op.
However, it is not necessary to change the insertion point. Unresolved
materializations (`UnrealizedConversionCastOp`) are built during
`buildUnresolvedArgumentMaterialization` or
`buildUnresolvedTargetMaterialization`. In the former case, the op is
inserted at the beginning of the block. In the latter case, only one
operand is supported in the dialect conversion, and the op is inserted
right after the definition of the SSA value. I.e., the
`UnrealizedConversionCastOp` is already inserted at the right place and
it is not necessary to change the insertion point for the resolved
materialization op.
Note: The IR change changes slightly because the
`unrealized_conversion_cast` ops at the beginning of a block are no
longer doubly-inverted (by setting the insertion to the beginning of the
block when inserting the `unrealized_conversion_cast` and again when
inserting the resolved conversion op). All affected test cases were
fixed by using `CHECK-DAG` instead of `CHECK`.
Also improve the quality of multiple test cases that did not check for
the correct operands.
Note: This commit is in preparation of decoupling the
argument/source/target materialization logic of the type converter from
the dialect conversion (to reduce its complexity and make that
functionality usable from a new dialect conversion driver).
This commit simplifies and improves documentation for the part of the
`ConversionPatternRewriter` API that deals with signature conversions.
There are now two public functions for signature conversion:
* `applySignatureConversion` converts a single block signature. This
function used to take a `Region *` (but converted only the entry block).
It now takes a `Block *`.
* `convertRegionTypes` converts all block signatures of a region.
`convertNonEntryRegionTypes` is removed because it is not widely used
and can easily be expressed with a call to `applySignatureConversion`
inside a loop. (See `Detensorize.cpp` for an example.)
Note: For consistency, `convertRegionTypes` could be renamed to
`applySignatureConversion` (overload) in the future. (Or
`applySignatureConversion` renamed to `convertBlockTypes`.)
Also clarify when a type converter and/or signature conversion object is
needed and for what purpose.
Internal code refactoring (NFC) of `ConversionPatternRewriterImpl` (the
part that deals with signature conversions). This part of the codebase
was quite convoluted and unintuitive.
From a functional perspective, this change is NFC. However, the public
API changes, thus not marking as NFC.
Note for LLVM integration: When you see
`applySignatureConversion(region, ...)`, replace with
`applySignatureConversion(region->front(), ...)`. In the unlikely case
that you see `convertNonEntryRegionTypes`, apply the same changes as
this commit did to `Detensorize.cpp`.
---------
Co-authored-by: Markus Böck <markus.boeck02@gmail.com>
This commit changes `OpBuilder::tryFold` to behave more similarly to
`Operation::fold`. Concretely, this ensures that even an in-place fold
returns `success`.
This is necessary to fix a bug in the dialect conversion that occurred
when an in-place folding made an operation legal. The dialect conversion
infrastructure did not check if the result of an in-place folding
legalized the operation and just went ahead and tried to apply pattern
anyways.
The added test contains a simplified version of a breakage we observed
downstream.
Before deleting the block we need to drop uses to the surrounding args.
If this is not performed dialect conversion failures can result in a
failure to remove args (despite the block having no remaining uses).
This commit adds two new notifications to `RewriterBase::Listener`:
* `notifyPatternBegin`: Called when a pattern application begins during
a greedy pattern rewrite or dialect conversion.
* `notifyPatternEnd`: Called when a pattern application finishes during
a greedy pattern rewrite or dialect conversion.
The listener infrastructure already provides a `notifyMatchFailure`
callback that notifies about the reason for a pattern match failure. The
two new notifications provide additional information about pattern
applications.
This change is in preparation of improving the handle update mechanism
in the `apply_conversion_patterns` transform op.
This commit adds listener support to the dialect conversion. Similarly
to the greedy pattern rewrite driver, an optional listener can be
specified in the configuration object.
Listeners are notified only if the dialect conversion succeeds. In case
of a failure, where some IR changes are first performed and then rolled
back, no notifications are sent.
Due to the fact that some kinds of rewrite are reflected in the IR
immediately and some in a delayed fashion, there are certain limitations
when attaching a listener; these are documented in `ConversionConfig`.
To summarize, users are always notified about all rewrites that
happened, but the notifications are sent all at once at the very end,
and not interleaved with the actual IR changes.
This change is in preparation improvements to
`transform.apply_conversion_patterns`, which currently invalidates all
handles. In the future, it can use a listener to update handles
accordingly, similar to `transform.apply_patterns`.
During block signature conversion, a new block is inserted and ops are
moved from the old block to the new block. This commit changes the
implementation such that ops are moved in bulk (`splice`) instead of
one-by-one; that's what `splitBlock` is doing.
This also makes it possible to pass the new block argument types
directly to `createBlock` instead of using `addArgument` (which bypasses
the rewriter). This doesn't change anything from a technical point of
view (there is no rewriter API for adding arguments at the moment), but
the implementation reads a bit nicer.
* `replaceOp` replaces all uses of the original op and erases the old
op.
* `replaceAllUsesWith` replaces all uses of the original op/value/block.
It does not erase any IR.
This commit renames `replaceOpWithIf` to `replaceUsesWithIf`.
`replaceOpWithIf` was a misnomer because the function never erases the
original op. Similarly, `replaceOpWithinBlock` is renamed to
`replaceUsesWithinBlock`. (No "operation replaced" is sent because the
op is not erased.)
Also improve comments.
notifyBlockInserted can be called when inserting a block in a region before
the op is built (like when building a scf::ForOp). This make us defensive
by checking the parent op before printing it.
The dialect conversion uses a `SingleEraseRewriter` to ensure that an
op/block is not erased twice. This can happen during the "commit" phase
when an unresolved materialization is inserted into a block and the
enclosing op is erased by the user. In that case, the unresolved
materialization should not be erased a second time later in the "commit"
phase.
This problem cannot happen during "rollback", so ops/block can be erased
directly without using the rewriter. With this change, the
`SingleEraseRewriter` is used only during "commit"/"cleanup". At that
point, the dialect conversion is guaranteed to succeed and no rollback
can happen. Therefore, it is not necessary to store the number of erased
IR objects (because we will never "reset" the rewriter to previous a
previous state).
When a block signature is converted during dialect conversion, a
`BlockTypeConversionRewrite` object is stored in the stack of rewrites.
Such an object represents multiple steps:
- Splitting the old block, i.e., creating a new block and moving all
operations over.
- Rewriting block arguments.
- Erasing the old block.
We have dedicated `IRRewrite` objects that represent "creating a block",
"moving an op" and "erasing a block". This commit reuses those rewrite
objects, so that there is less work to do in
`BlockTypeConversionRewrite::rollback` and
`BlockTypeConversionRewrite::commit`/`cleanup`.
Note: This change is in preparation of adding listener support to the
dialect conversion. The less work is done in a `commit` function, the
fewer notifications will have to be sent.
This commit adds a new `ConversionConfig` struct that allows users to
customize the dialect conversion. This configuration is similar to
`GreedyRewriteConfig` for the greedy pattern rewrite driver.
A few existing options are moved to this objects, simplifying the
dialect conversion API.
This is a re-upload of #82250. The Windows build breakage was fixed in #83768.
This reverts commit 60fbd60501.
This commit fixes a bug in a dialect conversion. Currently, when a block
is replaced via a signature conversion, the block is erased during the
"commit" phase. This is problematic because the block arguments may
still be referenced internal data structures of the dialect conversion
(`mapping`). Blocks should be treated same as ops: they should be erased
during the "cleanup" phase.
Note: The test case fails without this fix when running with ASAN, but
may pass when running without ASAN.
When splitting a block during a dialect conversion, a
`SplitBlockRewrite` object is stored in the dialect conversion state.
This commit removes `SplitBlockRewrite`. Instead, a combination of
`CreateBlockRewrite` and multiple `MoveOperationRewrite` is used.
This change simplifies the internal state of the dialect conversion and
is also needed to properly support listeners.
`RewriteBase::splitBlock` is now no longer virtual. All necessary
information for committing/rolling back a split block rewrite can be
deduced from `Listener::notifyBlockInserted` and
`Listener::notifyOperationInserted` (which is also called when moving an
operation).
The dialect conversion maintains sets of "ignored" and "replaced" ops.
This change simplifies the two sets, such that all nested ops are
included. (This was previously not the case and sometimes only the
parent op was included.)
This change allows for more aggressive assertions to prevent incorrect
rewriter API usage. E.g., accessing ops/blocks/regions within an erased
op.
A concrete example: I have seen conversion patterns in downstream
projects where an op is replaced with a new op, and the region of the
old op is afterwards inlined into the newly created op. This is invalid
rewriter API usage: ops that were replaced/erased should not be
accessed. Nested ops will be considered "ignored", even if they are
moved to a different region after the region's parent op was erased
(which is illegal API usage). Instead, create a new op, inline the
regions, then replace the old op with the new op.
#83023 fixed a performance regression related to "ignored" ops. This
broke some downstream projects that access ops after they were replaced
(an API violation). This change restores the original behavior before
#83023 (but without the performance regression), to give downstream
users more time to fix their code.
The dialect conversion does not directly erase ops that are
replaced/erased with a rewriter. Instead, the op stays in place and is
erased at the end if the dialect conversion succeeds. However, ops that
were replaced/erased are ignored from that point on.
#81757 introduced a compile time regression that made the check whether
an op is ignored or not more expensive. Whether an op is ignored or not
is queried many times throughout a dialect conversion, so the check must
be fast.
After this change, replaced ops are stored in the `ignoredOps` set. This
also simplifies the dialect conversion a bit.
This is a follow-up to #82333. It is possible that the target block of a
`BlockTypeConversionRewrite` is detached, so the `MLIRContext` cannot be
taken from the block.
`ConversionPatternRewriterImpl` no longer maintains a reference to the
respective `ConversionPatternRewriter`. An `MLIRContext` is sufficient.
This commit simplifies the internal state of
`ConversionPatternRewriterImpl`.
This commit adds a new `ConversionConfig` struct that allows users to
customize the dialect conversion. This configuration is similar to
`GreedyRewriteConfig` for the greedy pattern rewrite driver.
A few existing options are moved to this objects, simplifying the
dialect conversion API.
`ConversionPatternRewriter` objects should not be constructed outside of
dialect conversions. Some IR modifications performed through a
`ConversionPatternRewriter` are reflected in the IR in a delayed fashion
(e.g., only when the dialect conversion is guaranteed to succeed). Using
a `ConversionPatternRewriter` outside of the dialect conversion is
incorrect API usage and can bring the IR in an inconsistent state.
Migration guide: Use `IRRewriter` instead of
`ConversionPatternRewriter`.
This commit is a refactoring of the dialect conversion. The dialect
conversion maintains a list of "IR rewrites" that can be committed (upon
success) or rolled back (upon failure).
This commit turns the creation of unresolved materializations
(`unrealized_conversion_cast`) into `IRRewrite` objects. After this
commit, all steps in `applyRewrites` and `discardRewrites` are calls to
`IRRewrite::commit` and `IRRewrite::rollback`.
This commit is a refactoring of the dialect conversion. The dialect
conversion maintains a list of "IR rewrites" that can be committed (upon
success) or rolled back (upon failure).
Until now, the dialect conversion kept track of "op creation" in
separate internal data structures. This commit turns "op creation" into
an `IRRewrite` that can be committed and rolled back just like any other
rewrite. This commit simplifies the internal state of the dialect
conversion.
This commit is a refactoring of the dialect conversion. The dialect
conversion maintains a list of "IR rewrites" that can be committed (upon
success) or rolled back (upon failure).
Until now, op replacements and block argument replacements were kept
track in separate data structures inside the dialect conversion. This
commit turns them into `IRRewrite`s, so that they can be committed or
rolled back just like any other rewrite. This simplifies the internal
state of the dialect conversion.
Overview of changes:
* Add two new rewrite classes: `ReplaceBlockArgRewrite` and
`ReplaceOperationRewrite`. Remove the `OpReplacement` helper class; it
is now part of `ReplaceOperationRewrite`.
* Simplify `RewriterState`: `numReplacements` and `numArgReplacements`
are no longer needed. (Now being kept track of by `numRewrites`.)
* Add `IRRewrite::cleanup`. Operations should not be erased in `commit`
because they may still be referenced in other internal state of the
dialect conversion (`mapping`). Detaching operations is fine.
* `trackedOps` are now updated during the "commit" phase instead of
after applying all rewrites.
This commit is a refactoring of the dialect conversion. The dialect
conversion maintains a list of "IR rewrites" that can be committed (upon
success) or rolled back (upon failure).
Until now, the signature conversion of a block was only a "partial" IR
rewrite. Rollbacks were triggered via
`BlockTypeConversionRewrite::rollback`, but there was no
`BlockTypeConversionRewrite::commit` equivalent.
Overview of changes:
* Remove `ArgConverter`, an internal helper class that kept track of all
block type conversions. There is now a separate
`BlockTypeConversionRewrite` for each block type conversion.
* No more special handling for block type conversions. They are now
normal "IR rewrites", just like "block creation" or "block movement". In
particular, trigger "commits" of block type conversion via
`BlockTypeConversionRewrite::commit`.
* Remove `ArgConverter::notifyOpRemoved`. This function was used to
inform the `ArgConverter` that an operation was erased, to prevent a
double-free of operations in certain situations. It would be unpractical
to add a `notifyOpRemoved` API to `IRRewrite`. Instead, erasing
ops/block should go through a new `SingleEraseRewriter` (that is owned
by the `ConversionPatternRewriterImpl`) if there is chance of
double-free. This rewriter ignores `eraseOp`/`eraseBlock` if the
op/block was already freed.
This commit improves the block signature conversion API of the dialect
conversion.
There is the following comment in
`ArgConverter::applySignatureConversion`:
```
// If no arguments are being changed or added, there is nothing to do.
```
However, the implementation actually used to replace a block with a new
block even if the block argument types do not change (i.e., there is
"nothing to do"). This is fixed in this commit. The documentation of the
public `ConversionPatternRewriter` API is updated accordingly.
This commit also removes a check that used to *sometimes* skip a block
signature conversion if the block was already converted. This is not
consistent with the public `ConversionPatternRewriter` API; blocks
should always be converted, regardless of whether they were already
converted or not.
Block signature conversion also used to be silently skipped when the
specified block was detached. Instead of silently skipping, an assertion
is triggered. Attempting to convert a detached block (which is likely an
erased block) is invalid API usage.
When a `ModifyOperationRewrite` is committed, the operation may already
have been erased, so `OperationName` must be cached in the rewrite
object.
Note: This will no longer be needed with #81757, which adds a "cleanup"
method to `IRRewrite`.
* When converting a block signature, `ArgConverter` creates a new block
with the new signature and moves all operation from the old block to the
new block. The new block is temporarily inserted into a region that is
stored in `regionMapping`. The old block is not yet deleted, so that the
conversion can be rolled back. `regionMapping` is not needed. Instead of
moving the old block to a temporary region, it can just be unlinked.
Block erasures are handles in the same way in the dialect conversion.
* `regionToConverter` is a mapping from regions to type converter. That
field is never accessed within `ArgConverter`. It should be stored in
`ConversionPatternRewriterImpl` instead.
* `convertedBlocks` is not needed. Old blocks are already stored in
`ConvertedBlockInfo`.
The dialect conversion rolls back in-place op modifications upon
failure. Rolling back modifications of attributes is already supported,
but there was no support for properties until now.
This commit simplifies the internal state of the dialect conversion. A
separate field for the previous state of in-place op modifications is no
longer needed.
Add a new rewrite class for "operation movements". This rewrite class
can roll back `moveOpBefore` and `moveOpAfter`.
`RewriterBase::moveOpBefore` and `RewriterBase::moveOpAfter` is no
longer virtual. (The dialect conversion can gather all required
information for rollbacks from listener notifications.)
Throughout the rewrite process, the dialect conversion maintains a list
of "block actions" that can be rolled back upon failure. This commit
encapsulates the existing block actions into separate classes, making it
easier to add additional actions in the future.
This commit also renames "block actions" to "IR rewrites". In a
subsequent commit, an "operation rewrite" class that allows rolling back
movements of single operations is added. This is to support
`moveOpBefore` in the dialect conversion.
Rewrites have two methods: `commit()` commits an action. It can no
longer be rolled back afterwards. `rollback()` undoes a rewrite. It can
no longer be committed afterwards.
