| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
GreedyPatternRewriteDriver.cpp (NFC)
|
|
(#153961)
Also improve a bit the LDBG() implementation
|
|
rewriter (#153957)
Operations like:
%add = arith.addi %add, %add : i64
are legal in unreachable code. Unfortunately many patterns would be
unsafe to apply on such IR and can lead to crashes or infinite loops. To
avoid this we can remove unreachable blocks before attempting to apply
patterns.
We may have to do this also whenever the CFG is changed by a pattern, it
is left up for future work right now.
Fixes #153732
|
|
building this file would fail when
MLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS = 1
Signed-off-by: dan <danimal197@gmail.com>
|
|
These are identified by misc-include-cleaner. I've filtered out those
that break builds. Also, I'm staying away from llvm-config.h,
config.h, and Compiler.h, which likely cause platform- or
compiler-specific build failures.
|
|
This is similar to other configuration objects used across MLIR.
Rename some fields to better reflect that they are no longer booleans.
Reland 04d261101b4f229189463136a794e3e362a793af / #132253.
|
|
This reverts commit 63b8f1c9482ed0a964980df4aed89bef922b8078.
Buildbot failure:
https://lab.llvm.org/buildbot/#/builders/172/builds/12083/steps/5/logs/stdio
I've reproduced the error with a release build (-DCMAKE_BUILD_TYPE=Release).
|
|
This is similar to other configuration objects used across MLIR.
|
|
These have been deprecated for over two years now in favor of free
functions.
See the relevant discourse thread:
https://discourse.llvm.org/t/preferred-casting-style-going-forward/68443
and the deprecation notice: https://mlir.llvm.org/deprecation/.
|
|
DenseSet, SmallPtrSet, SmallSet, SetVector, and StringSet recently
gained C++23-style insert_range. This patch replaces:
Dest.insert(Src.begin(), Src.end());
with:
Dest.insert_range(Src);
This patch does not touch custom begin like succ_begin for now.
|
|
Currently, when `GreedyPatternRewriteDriver` fails, the log output
contains nested failure messages:
```bash
} -> failure : pattern failed to match
} -> failure : pattern failed to match
```
This may seem redundant, but these messages refer to different aspects
of the pattern application logic. This patch clarifies the distinction
by separately logging:
* Success/failure for a specific pattern (e.g., "_this pattern_ failed
to match on the Op currently being processed").
* Success/failure for an operation as a whole (e.g., "_all patterns_
failed to match the Op currently being processed").
Before (example with success):
```bash
Processing operation : (...) {
* Pattern (...) -> ()' {
Trying to match "..."
** Match Failure : (...)
} -> failure : pattern failed to match
* Pattern (...) -> ()' {
Trying to match "..."
} -> success : pattern applied successfully
} -> success : pattern matched
```
After (example with success):
```bash
Processing operation : (...) {
* Pattern (...) -> ()' {
Trying to match "..."
** Match Failure : (...)
} -> failure : pattern failed to match
* Pattern (...) -> ()' {
Trying to match "..."
} -> success : pattern applied successfully
} -> success : at least one pattern matched
```
This improves log clarity, making it easier to distinguish pattern-level
failures from operation-level outcomes.
|
|
Note that PointerUnion::{is,get} have been soft deprecated in
PointerUnion.h:
// FIXME: Replace the uses of is(), get() and dyn_cast() with
// isa<T>, cast<T> and the llvm::dyn_cast<T>
I'm not touching PointerUnion::dyn_cast for now because it's a bit
complicated; we could blindly migrate it to dyn_cast_if_present, but
we should probably use dyn_cast when the operand is known to be
non-null.
|
|
The greedy rewriter is used in many different flows and it has a lot of
convenience (work list management, debugging actions, tracing, etc). But
it combines two kinds of greedy behavior 1) how ops are matched, 2)
folding wherever it can.
These are independent forms of greedy and leads to inefficiency. E.g.,
cases where one need to create different phases in lowering and is
required to applying patterns in specific order split across different
passes. Using the driver one ends up needlessly retrying folding/having
multiple rounds of folding attempts, where one final run would have
sufficed.
Of course folks can locally avoid this behavior by just building their
own, but this is also a common requested feature that folks keep on
working around locally in suboptimal ways.
For downstream users, there should be no behavioral change. Updating
from the deprecated should just be a find and replace (e.g., `find ./
-type f -exec sed -i
's|applyPatternsAndFoldGreedily|applyPatternsGreedily|g' {} \;` variety)
as the API arguments hasn't changed between the two.
|
|
This is a heavy process, and it can trigger a massive explosion in
adding block arguments. While potentially reducing the code size, the
resulting merged blocks with arguments are hiding some of the def-use
chain and can even hinder some further analyses/optimizations: a merge
block does not have it's own path-sensitive context, instead the context
is merged from all the predecessors.
Previous behavior can be restored by passing:
{test-convergence region-simplify=aggressive}
to the canonicalize pass.
|
|
instead of inheritance (#92785)
This commit simplifies the design of the `GreedyPatternRewriterDriver`
class. This class used to inherit from both `PatternRewriter` and
`RewriterBase::Listener` and then attached itself as a listener.
In the new design, the class has a `PatternRewriter` field instead of
inheriting from `PatternRewriter`, which is generally perferred in
object-oriented programming.
---------
Co-authored-by: Markus Böck <markus.boeck02@gmail.com>
|
|
GreedyPatternRewriteDriver worklist (NFC) (#88148)
This is a common anti-pattern (any volunteer for a clang-tidy check?).
This does not show real word significant impact though.
|
|
I believe the existing check to determine if an operand should be added
is incorrect: `operand.use_empty() || operand.hasOneUse()`. This is
because these checks do not take into account the fact that the op is
being deleted. It hasn't been deleted yet, so `operand.use_empty()`
cannot be true, and `operand.hasOneUse()` may be true if the op being
deleted is the only user of the operand and it only uses it once, but it
will fail if the operand is used more than once (e.g. something like
`add %0, %0`).
Instead, check if the op being deleted is the only _user_ of the
operand. If so, add the operand to the worklist.
Fixes #86765
|
|
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.
|
|
Rename listener callback names:
* `notifyOperationRemoved` -> `notifyOperationErased`
* `notifyBlockRemoved` -> `notifyBlockErased`
The current callback names are misnomers. The callbacks are triggered
when an operation/block is erased, not when it is removed (unlinked).
E.g.:
```c++
/// Notify the listener that the specified operation is about to be erased.
/// At this point, the operation has zero uses.
///
/// Note: This notification is not triggered when unlinking an operation.
virtual void notifyOperationErased(Operation *op) {}
```
This change is in preparation of adding listener support to the dialect
conversion. The dialect conversion internally unlinks IR before erasing
it at a later point of time. There is an important difference between
"remove" and "erase". Lister callback names should be accurate to avoid
confusion.
|
|
There are two `notifyMatchFailure` methods: one in the rewriter and one
in the listener. The one in the rewriter notifies the listener and
returns "failure" for convenience. The one in the listener should not
return anything; it is just a notification. It can currently be abused
to return "success" from the rewriter function. That would be a
violation of the rewriter API rules.
Also make sure that the listener is always notified about match
failures, not just with `NDEBUG`. The current implementation is
consistent: one `notifyMatchFailure` overload notifies only in debug
mode and another one notifies all the time.
|
|
The greedy pattern rewrite driver has multiple "expensive checks" to
detect invalid rewrite pattern API usage. As part of these checks, it
computes fingerprints for every op that is in scope, and compares the
fingerprints before and after an attempted pattern application.
Until now, each computed fingerprint took into account all nested
operations. That is quite expensive because it walks the entire IR
subtree. It is also redundant in the expensive checks because we already
compute a fingerprint for every op.
This commit significantly improves the running time of the "expensive
checks" in the greedy pattern rewrite driver.
|
|
After #75103, `MLPrgramTransforms` depends on `BufferizationDialect`.
Also fix an unrelated compile error in `GreedyPatternRewriteDriver.cpp`.
(This was not failing on CI. I may be running an old compiler locally.)
|
|
This change makes the callback consistent with
`notifyOperationInserted`: both now notify about IR insertion, not IR
creation. See also #78988.
This change also simplifies the dialect conversion: it is no longer
necessary to override the `inlineRegionBefore` method. All information
that is necessary for rollback is provided with the
`notifyBlockInserted` callback.
|
|
The pattern rewriter documentation states that "*all* IR mutations [...]
are required to be performed via the `PatternRewriter`." This commit
adds two functions that were missing from the rewriter API:
`moveOpBefore` and `moveOpAfter`.
After an operation was moved, the `notifyOperationInserted` callback is
triggered. This allows listeners such as the greedy pattern rewrite
driver to react to IR changes.
This commit narrows the discrepancy between the kind of IR modification
that can be performed and the kind of IR modifications that can be
listened to.
|
|
There is already a "block inserted" notification (in
`OpBuilder::Listener`), so there should also be a "block removed"
notification.
The purpose of this change is to make the listener API more mature.
There is currently a gap between what kind of IR changes can be made and
what IR changes can be listened to. At the moment, the only way to
inform listeners about "block removal" is to send a manual
`notifyOperationModified` for the parent op (e.g., by wrapping the
`eraseBlock(b)` method call in `updateRootInPlace(b->getParentOp())`).
This tells the listener that *something* has changed, but it is somewhat
of an API abuse.
|
|
encapsulation (#78175)
This change moves most IR verification logic (which is part of the
expensive checks) into `DebugFingerPrints` and renames the struct to
`ExpensiveChecks`. This isolates the debugging logic better from the
remaining code.
This commit also removes a redundant check: the IR is no longer verified
after a failed pattern application. We already assert that the IR did
not change. (We know that the IR was valid before the attempted pattern
application.)
|
|
Similar to successful pattern applications, dump the rewritten IR after
each successful folding when running with `-debug`.
|
|
Make GreedyPatternRewriteDriver handle failures of `materializeConstant`
gracefully. Previously it was not checking whether the returned op was
null and crashing. This PR handles it similarly to how OperationFolder
does it.
|
|
iterations (#75897)
The `GreedyPatternRewriteDriver` tries to iteratively fold ops and apply
rewrite patterns to ops. It has special handling for constants: they are
CSE'd and sometimes moved to parent regions to allow for additional
CSE'ing. This happens in `OperationFolder`.
To allow for efficient CSE'ing, `OperationFolder` maintains an internal
lookup data structure to find the existing constant ops with the same
value for each `IsolatedFromAbove` region:
```c++
/// A mapping between an insertion region and the constants that have been
/// created within it.
DenseMap<Region *, ConstantMap> foldScopes;
```
Rewrite patterns are allowed to modify operations. In particular, they
may move operations (including constants) from one region to another
one. Such an IR rewrite can make the above lookup data structure
inconsistent.
We encountered such a bug in a downstream project. This bug materialized
in the form of an op that uses the result of a constant op from a
different `IsolatedFromAbove` region (that is not accessible).
This commit changes the behavior of the `GreedyPatternRewriteDriver`
such that `OperationFolder` is used to CSE constants at the beginning of
each iteration (as the worklist is populated), but no longer during an
iteration. `OperationFolder` is no longer used after populating the
worklist, so we do not have to care about inconsistent state in the
`OperationFolder` due to IR rewrites. The `GreedyPatternRewriteDriver`
now performs the op folding by itself instead of calling
`OperationFolder::tryToFold`.
This change changes the order of constant ops in test cases, but not the
region in which they appear. All broken test cases were fixed by turning
`CHECK` into `CHECK-DAG`.
Alternatives considered: The state of `OperationFolder` could be
partially invalidated with every `notifyOperationModified` notification.
That is more fragile than the solution in this commit because incorrect
rewriter API usage can lead to missing notifications and hard-to-debug
`IsolatedFromAbove` violations. (It did not fix the above mention bug in
a downstream project, which could be due to incorrect rewriter API usage
or due to another conceptual problem that I missed.) Moreover, ops are
frequently getting modified during a greedy pattern rewrite, so we would
likely keep invalidating large parts of the state of `OperationFolder`
over and over.
Migration guide: Turn `CHECK` into `CHECK-DAG` in test cases. Constant
ops are no longer folded during a greedy pattern rewrite. If you rely on
folding (and rematerialization) of constant ops during a greedy pattern
rewrite, turn the folder into a pattern.
|
|
This commit adds an additional "expensive check" that verifies the IR
before starting a greedy pattern rewriter, after every pattern
application and after every folding. (Only if
`MLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS` is set.)
It also adds an assertion that the `scope` region (part of
`GreedyRewriteConfig`) is not being erased as part of the greedy pattern
rewrite. That would break the scoping mechanism and the expensive
checks.
This commit does not fix any patterns, this is done in separate commits.
|
|
worklist management code (#74796)
Do not add the previous users of replaced ops to the worklist during
`notifyOperationReplaced`.
The previous users are modified inplace as part of
`PatternRewriter::replaceOp`, which calls
`PatternRewriter::replaceAllUsesWith`. The latter function updates all
users with `updateRootInPlace`, which already puts all previous users of
the replaced op on the worklist. No further worklist management work is
needed in the `notifyOperationReplaced` callback.
|
|
When cloning an op, the `notifyOperationInserted` callback is triggered
for all nested ops. Similarly, the `notifyOperationRemoved` callback
should be triggered for all nested ops when removing an op.
Listeners may inspect the IR during a `notifyOperationRemoved` callback.
Therefore, when multiple ops are removed in a single
`RewriterBase::eraseOp` call, the notifications must be triggered in an
order in which the ops could have been removed one-by-one:
* Op removals must be interleaved with `notifyOperationRemoved`
callbacks. A callback is triggered right before the respective op is
removed.
* Ops are removed post-order and in reverse order. Other traversal
orders could delete an op that still has uses. (This is not avoidable in
graph regions and with cyclic block graphs.)
Differential Revision: Imported from https://reviews.llvm.org/D144193.
|
|
`applyPatternsAndFoldGreedily`
This allows users of `applyPatternsAndFoldGreedily` to detect if any MLIR changes have occurred. An example use-case is where we expect the `applyPatternsAndFoldGreedily` to change the IR and want to validate that it indeed does change it.
Differential Revision: https://reviews.llvm.org/D153986
|
|
Instead of always taking the last op from the worklist, take a random one. For testing/debugging purposes only. This feature can be used to ensure that lowering pipelines work correctly regardless of the order in which ops are processed by the GreedyPatternRewriteDriver.
The randomizer can be enabled by setting a numeric `MLIR_GREEDY_REWRITE_RANDOMIZER_SEED` option.
Note: When enabled, 27 tests are currently failing. Partly because FileCheck tests are looking for exact IR.
Discussion: https://discourse.llvm.org/t/discussion-fuzzing-pattern-application/67911
Differential Revision: https://reviews.llvm.org/D142447
|
|
Encapsulate all worklist-related functionality in a separate `Worklist` class. This makes the remaining code more readable and allows for custom worklist implementations (e.g., a randomized worklist for fuzzing pattern application: D142447).
Differential Revision: https://reviews.llvm.org/D151345
|
|
Boolean compiler flags (such as `DMLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS`) show up in `mlir-config.h` as preprocessor defines that are either 0 or 1. Use `#if` instead of `#ifdef`.
This should have been part of D144552.
|
|
Compute operation finger prints to detect incorrect API usage in RewritePatterns. Does not work for dialect conversion patterns.
Detect patterns that:
* Returned `failure` but changed the IR.
* Returned `success` but did not change the IR.
* Inserted/removed/modified ops, bypassing the rewriter. Not all cases are detected.
These new checks are quite expensive, so they are only enabled with `-DMLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS=ON`. Failures manifest as fatal errors (`llvm::report_fatal_error`) or crashes (accessing deallocated memory). To get better debugging information, run `mlir-opt -debug` (to see which pattern is broken) with ASAN (to see where memory was deallocated).
Differential Revision: https://reviews.llvm.org/D144552
|
|
Do not duplicate code that is performing actual work, put debug code around it.
Differential Revision: https://reviews.llvm.org/D151207
|
|
Differential Revision: https://reviews.llvm.org/D149101
|
|
The previous implementation did not notify the attached listener.
Reviewed By: springerm
Differential Revision: https://reviews.llvm.org/D145049
|
|
Remove the IR modification callbacks from `OperationFolder`. Instead, an optional `RewriterBase::Listener` can be specified.
* `processGeneratedConstants` => `notifyOperationCreated`
* `preReplaceAction` => `notifyOperationReplaced`
This simplifies the GreedyPatternRewriterDriver because we no longer need special handling for IR modifications due to op folding.
A folded operation is now enqueued on the GreedyPatternRewriteDriver's worklist if it was modified in-place. (There may be new patterns that apply after folding.)
Also fixes a bug in `TestOpInPlaceFold::fold`. The folder could previously be applied over and over and did not return a "null" OpFoldResult if the IR was not modified. (This is similar to a pattern that returns `success` without modifying IR; it can trigger an infinite loop in the GreedyPatternRewriteDriver.)
Differential Revision: https://reviews.llvm.org/D144463
|
|
This callback is triggered by `finalizeRootUpdate`. This allows listeners to listen for in-place op modifications without creating a new RewriterBase subclass.
Differential Revision: https://reviews.llvm.org/D143380
|
|
Allow an optional `RewriterBase::Listener` to be attached to greedy pattern rewrites, so that clients can listen for IR modifications.
Differential Revision: https://reviews.llvm.org/D143340
|
|
```
OpBuilder OpBuilder::Listener
^ ^
| |
RewriterBase RewriterBase::Listener
```
* Clients can listen to IR modifications with `RewriterBase::Listener`.
* `RewriterBase` no longer inherits from `OpBuilder::Listener`.
* Only a single listener can be registered at the moment (same as `OpBuilder`).
RFC: https://discourse.llvm.org/t/rfc-listeners-for-rewriterbase/68198
Differential Revision: https://reviews.llvm.org/D143339
|
|
Top-level ModuleOps cannot be transformed with the GreedyPatternRewriteDriver since D141945 because they do not have an enclosing region that could be used as a scope. Make the scope optional inside GreedyPatternRewriteDriver, so that top-level ops can be processed when they are on the initial list of ops.
Note: This does not allow users to bypass the scoping mechanism by setting `config.scope = nullptr`.
Fixes #60462.
Differential Revision: https://reviews.llvm.org/D143151
|
|
multi-op-based drivers
Deduplicate large parts of the worklist processing (`GreedyPatternRewriteDriver::processWorklist`).
The new class hierarchy is as follows:
```
GreedyPatternRewriteDriver (abstract)
^
|
-----------------------------------
| |
RegionPatternRewriteDriver MultiOpPatternRewriteDriver
```
Also update the Markdown documentation.
Differential Revision: https://reviews.llvm.org/D141396
|
|
GreedyPatternRewriteDriver
`strictMode` is moved to GreedyRewriteConfig to simplify the API and state of rewriter classes. The region-based GreedyPatternRewriteDriver now also supports strict mode.
MultiOpPatternRewriteDriver becomes simpler: fewer method must be overridden.
Differential Revision: https://reviews.llvm.org/D142623
|
|
The multi-op entry point now also takes a GreedyPatternRewriteConfig and respects config.maxNumRewrites. The scope is also a part of the config now.
Differential Revision: https://reviews.llvm.org/D142614
|
|
The rewrite driver is typically applied to a single region or all regions of the same op. There is no longer an overload to apply the rewrite driver to a list of regions.
This simplifies the rewrite driver implementation because the scope is now a single region as opposed to a list of regions.
Note: This change is not NFC because `config.maxIterations` and `config.maxNumRewrites` is now counted for each region separately. Furthermore, worklist filtering (`scope`) is now applied to each region separately.
Differential Revision: https://reviews.llvm.org/D142611
|
|
Less mutable state, more `const`. This is to address a concern about complexity of state in D140304.
Differential Revision: https://reviews.llvm.org/D141949
|
