Two related assertion failures in DeadCodeAnalysis when processing
OpenACC operations:
1. visitRegionBranchEdges (issue #187972): When a RegionSuccessor refers
to an empty region (no blocks), calling getSuccessor()->front()
dereferences a sentinel ilist iterator, crashing with
"\!NodePtr->isKnownSentinel()". Fix: skip successors whose region is
empty.
2. isRegionOrCallableReturn (issue #188408): When iterating over ops in
a nested acc region whose blocks do not have a required terminator,
Block::getTerminator() is called without first checking
mightHaveTerminator(), triggering "Assertion `mightHaveTerminator()'
failed". Fix: guard the getTerminator() call with mightHaveTerminator().
Fixes#187972, #188408
Assisted-by: Claude Code
This commit simplifies the design of the `RegionBranchOpInterface`. The
property of being a successor input is now independent of the region
branch point.
There is a new API for querying successor inputs:
`RegionBranchOpInterface::getSuccessorInputs(RegionSuccessor)`. Note
that this function does **not** take a `RegionBranchPoint` as parameter.
The `RegionSuccessor` API is now also simpler: it no longer stores
successor inputs. A region successor is simply `Region *`, wrapped
around a convenience API.
Note: This commit is mostly mechanical. Analyses / transformations that
build on top of the `RegionBranchOpInterface` (e.g.,
`visitNonControlFlowArguments` API) can likely be simplified in
follow-up commits.
Note for LLVM integration: Split
`RegionBranchOpInterface::getSuccessorRegion` implementations into two
functions: `getSuccessorRegion` and `getSuccessorInputs. (There are many
examples in this commit.)
RFC:
https://discourse.llvm.org/t/rfc-simplify-regionbranchopinterface-separate-successor-inputs-from-region-successor/89420/7
This is still somehow a WIP, we have some issues with this interface
that are not trivial to solve. This patch tries to make the concepts of
RegionBranchPoint and RegionSuccessor more robust and aligned with their
definition:
- A `RegionBranchPoint` is either the parent (`RegionBranchOpInterface`)
op or a `RegionBranchTerminatorOpInterface` operation in a nested
region.
- A `RegionSuccessor` is either one of the nested region or the parent
`RegionBranchOpInterface`
Some new methods with reasonnable default implementation are added to
help resolving the flow of values across the RegionBranchOpInterface.
It is still not trivial in the current state to walk the def-use chain
backward with this interface. For example when you have the 3rd block
argument in the entry block of a for-loop, finding the matching operands
requires to know about the hidden loop iterator block argument and where
the iterargs start. The API is designed around forward-tracking of the
chain unfortunately.
Try to reland #161575 ; I suspect a buildbot incremental build issue.
This is still somehow a WIP, we have some issues with this interface
that are not trivial to solve. This patch tries to make the concepts of
RegionBranchPoint and RegionSuccessor more robust and aligned with their
definition:
- A `RegionBranchPoint` is either the parent (`RegionBranchOpInterface`)
op or a `RegionBranchTerminatorOpInterface` operation in a nested
region.
- A `RegionSuccessor` is either one of the nested region or the parent
`RegionBranchOpInterface`
Some new methods with reasonnable default implementation are added to
help resolving the flow of values across the RegionBranchOpInterface.
It is still not trivial in the current state to walk the def-use chain
backward with this interface. For example when you have the 3rd block
argument in the entry block of a for-loop, finding the matching operands
requires to know about the hidden loop iterator block argument and where
the iterargs start. The API is designed around forward-tracking of the
chain unfortunately.
* Remove `getOperandValuesImpl` since its only used once.
* Extract common logic from
`DeadCodeAnalysis::visitRegion{BranchOperation,Terminator}` into a new
function `DeadCodeAnalysis::visitRegionBranchEdges`.
In particular, both functions do the following:
* Detect live region branch edges (similar to CFGEdge);
* For each edge, mark the successor program point as executable (so that
subsequent program gets visited);
* For each edge, store the information of the predecessor op and
arguments (so that other analyses know what states to join into the
successor program point).
One caveat is that, before this PR, in `visitRegionTerminator`, the
successor program point is only marked as live if it is the start of a
block; after this PR, the successor program point is consistently marked
as live regardless what it is, which makes the behavior equal to
`visitBranchOperation`. This minor fix improves consistency, but at this
point it is still NFC, because the rest of the dataflow analysis
framework only cares about liveness at block level, and the liveness
information in the middle of a block isn't read anyway. This probably
will change once
[early-exits](https://discourse.llvm.org/t/rfc-region-based-control-flow-with-early-exits-in-mlir/76998)
are supported.
We are using the symbol table machinery to lookup for a callable, but
when the analysis scope if a function, such lookup will resolve outside
of the scope. This can lead to race-condition issues since other passes
may operate in parallel on the sibling functions.
The callable would be discarded right after the lookup (we check the
analysis scope), so avoiding the lookup is NFC.
For the DataFlow solver, we're looking at the top-level operation, and
if it isn't a SymbolTable we disable the interprocedural optimization in
the solver config directly.
This strategy isn't NFC but seems reasonnable and does not encounter any
change in behavior in practice in tree.
Fix#154948
This is improving the debug output:
- avoid printing pointers, print ops without regions in general.
- skip extra new-lines in the output
- minor other consistency aspects.
Dead code analysis crashed because a symbol that is called/used didn't appear in the symbol
table.
This patch fixes this by adding a nullptr check after symbol table lookup.
The concept of a 'program point' in the original data flow framework is
ambiguous. It can refer to either an operation or a block itself. This
representation has different interpretations in forward and backward
data-flow analysis. In forward data-flow analysis, the program point of
an operation represents the state after the operation, while in backward
data flow analysis, it represents the state before the operation. When
using forward or backward data-flow analysis, it is crucial to carefully
handle this distinction to ensure correctness.
This patch refactors the definition of program point, unifying the
interpretation of program points in both forward and backward data-flow
analysis.
How to integrate this patch?
For dense forward data-flow analysis and other analysis (except dense
backward data-flow analysis), the program point corresponding to the
original operation can be obtained by `getProgramPointAfter(op)`, and
the program point corresponding to the original block can be obtained by
`getProgramPointBefore(block)`.
For dense backward data-flow analysis, the program point corresponding
to the original operation can be obtained by
`getProgramPointBefore(op)`, and the program point corresponding to the
original block can be obtained by `getProgramPointAfter(block)`.
NOTE: If you need to get the lattice of other data-flow analyses in
dense backward data-flow analysis, you should still use the dense
forward data-flow approach. For example, to get the Executable state of
a block in dense backward data-flow analysis and add the dependency of
the current operation, you should write:
``getOrCreateFor<Executable>(getProgramPointBefore(op),
getProgramPointBefore(block))``
In case above, we use getProgramPointBefore(op) because the analysis we
rely on is dense backward data-flow, and we use
getProgramPointBefore(block) because the lattice we query is the result
of a non-dense backward data flow computation.
related dsscussion:
https://discourse.llvm.org/t/rfc-unify-the-semantics-of-program-points/80671/8
corresponding PSA:
https://discourse.llvm.org/t/psa-program-point-semantics-change/81479
Allow customization of the `resolveCallable` method in the
`CallOpInterface`. This change allows for operations implementing this
interface to provide their own logic for resolving callables.
- Introduce the `resolveCallable` method, which does not include the
optional symbol table parameter. This method replaces the previously
existing extra class declaration `resolveCallable`.
- Introduce the `resolveCallableInTable` method, which incorporates the
symbol table parameter. This method replaces the previous extra class
declaration `resolveCallable` that used the optional symbol table
parameter.
This patch distinguishes between program points and lattice anchors in
data flow analysis, where lattice anchors represent locations where a
lattice can be attached, while program points denote points in program
execution.
Related discussions:
https://discourse.llvm.org/t/rfc-unify-the-semantics-of-program-points/80671/8
Fix for a crash reported in #64975.
The crash occurs in the cast located
[here](ece5dd101c/mlir/lib/Analysis/DataFlow/DeadCodeAnalysis.cpp (L262))
because `llvm.unreachable` doesn't implement
`RegionBranchTerminatorOpInterface`.
The crash is caused by `DeadCodeAnalysis` assuming that
`isa<RegionBranchOpInterface>(op->getParentOp())` implies
`isa<RegionBranchTerminatorOpInterface>(op)` in
`DeadCodeAnalysis::visit()`.
This patch tried to fix this by enabling the analysis to proceed
regardless of whether `op` is a `RegionBranchTerminatorOpInterface`.
The PredecessorState in dead code analysis does not register
zero-operand returns as predecessors of their corresponding call ops.
This causes bugs with dense dataflow analyses that use dead code
analysis to query for the predecessors of CallOpInterfaces.
This was reasonable for sparse analyses, but isn't for dense ones.
The `RegionBranchOpInterface` had a few fundamental issues caused by the API design of `getSuccessorRegions`.
It always required passing values for the `operands` parameter. This is problematic as the operands parameter actually changes meaning depending on which predecessor `index` is referring to. If coming from a region, you'd have to find a `RegionBranchTerminatorOpInterface` in that region, get its operand count, and then create a `SmallVector` of that size.
This is not only inconvenient, but also error-prone, which has lead to a bug in the implementation of a previously existing `getSuccessorRegions` overload.
Additionally, this made the method dual-use, trying to serve two different use-cases: 1) Trying to determine possible control flow edges between regions and 2) Trying to determine the region being branched to based on constant operands.
This patch fixes these issues by changing the interface methods and adding new ones:
* The `operands` argument of `getSuccessorRegions` has been removed. The method is now only responsible for returning possible control flow edges between regions.
* An optional `getEntrySuccessorRegions` method has been added. This is used to determine which regions are branched to from the parent op based on constant operands of the parent op. By default, it calls `getSuccessorRegions`. This is analogous to `getSuccessorForOperands` from `BranchOpInterface`.
* Add `getSuccessorRegions` to `RegionBranchTerminatorOpInterface`. This is used to get the possible successors of the terminator based on constant operands. By default, it calls the containing `RegionBranchOpInterface`s `getSuccessorRegions` method.
* `getSuccessorEntryOperands` was renamed to `getEntrySuccessorOperands` for consistency.
Differential Revision: https://reviews.llvm.org/D157506
In the MLIR dataflow analysis framework, when an `AnalysisState` is updated, it's dependents are enqueued to be visited.
Currently, there are two ways dependents are managed:
* `AnalysisState::dependents` stores a list of dependents. `DataFlowSolver::propagateIfChanged()` reads this list and enqueues them to the worklist.
* `AnalysisState::onUpdate()` allows custom logic to enqueue more to the worklist. This is called by `DataFlowSolver::propagateIfChanged()`.
This cleanup diff consolidates the two into `AnalysisState::onUpdate()`. This way, `DataFlowSolver` does not need to know the detail about `AnalysisState::dependents`, and the logic of dependency management is entirely handled by `AnalysisState`.
Reviewed By: Mogball
Differential Revision: https://reviews.llvm.org/D154170
The MLIR classes Type/Attribute/Operation/Op/Value support
cast/dyn_cast/isa/dyn_cast_or_null functionality through llvm's doCast
functionality in addition to defining methods with the same name.
This change begins the migration of uses of the method to the
corresponding function call as has been decided as more consistent.
Note that there still exist classes that only define methods directly,
such as AffineExpr, and this does not include work currently to support
a functional cast/isa call.
Context:
- https://mlir.llvm.org/deprecation/ at "Use the free function variants
for dyn_cast/cast/isa/…"
- Original discussion at https://discourse.llvm.org/t/preferred-casting-style-going-forward/68443
Implementation:
This patch updates all remaining uses of the deprecated functionality in
mlir/. This was done with clang-tidy as described below and further
modifications to GPUBase.td and OpenMPOpsInterfaces.td.
Steps are described per line, as comments are removed by git:
0. Retrieve the change from the following to build clang-tidy with an
additional check:
main...tpopp:llvm-project:tidy-cast-check
1. Build clang-tidy
2. Run clang-tidy over your entire codebase while disabling all checks
and enabling the one relevant one. Run on all header files also.
3. Delete .inc files that were also modified, so the next build rebuilds
them to a pure state.
```
ninja -C $BUILD_DIR clang-tidy
run-clang-tidy -clang-tidy-binary=$BUILD_DIR/bin/clang-tidy -checks='-*,misc-cast-functions'\
-header-filter=mlir/ mlir/* -fix
rm -rf $BUILD_DIR/tools/mlir/**/*.inc
```
Differential Revision: https://reviews.llvm.org/D151542
This is part of an effort to migrate from llvm::Optional to
std::optional. 22426110c5ef changed the way mlir-tblgen generates .inc
files, emitting std::optional when an Optional attribute is specified in
a .td file. It also changed several .td files hard-coding llvm::Optional
to use std::optional. However, the patch excluded a few .td files in
SPIRV and Bufferization hard-coding llvm::Optional. This patch fixes
that defect, and after this patch, references to llvm::Optional in .cpp
and .h files can be replaced mechanically.
See also: https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716
Signed-off-by: Ramkumar Ramachandra <r@artagnon.com>
Differential Revision: https://reviews.llvm.org/D140329
Currently, for sparse analyses, we always store a `Optional<ValueT>` in each lattice element. When it's `None`, we consider the lattice element as `uninitialized`.
However:
* Not all lattices have an `uninitialized` state. For example, `Executable` and `PredecessorState` have default values so they are always initialized.
* In dense analyses, we don't have the concept of an `uninitialized` state.
Given these inconsistencies, this patch removes `Lattice::isUninitialized()`. Individual analysis states are now default-constructed. If the default state of an analysis can be considered as "uninitialized" then this analysis should implement the following logic:
* Special join rule: `join(uninitialized, any) == any`.
* Special bail out logic: if any of the input states is uninitialized, exit the transfer function early.
Depends On D132086
Reviewed By: Mogball
Differential Revision: https://reviews.llvm.org/D132800
When dead-code analysis is run at the scope of a function, call ops to
other functions at the same level were being marked as unreachable,
since the analysis optimistically assumes the call op to have no known
predecessors and that all predecessors are known, but the callee would
never get visited.
This patch fixes the bug by checking if a referenced function is above
the top-level op of the analysis, and is thus considered an external
callable.
Fixes#56830
Reviewed By: zero9178
Differential Revision: https://reviews.llvm.org/D130829
Integer range inference has been swapped to the new framework. The integer value range lattices automatically updates the corresponding constant value on update.
Depends on D127173
Reviewed By: krzysz00, rriddle
Differential Revision: https://reviews.llvm.org/D128866
This patch introduces a (forward) sparse data-flow analysis implemented with the data-flow analysis framework. The analysis interacts with liveness information that can be provided by dead-code analysis to be conditional. This patch re-implements SCCP using dead-code analysis and (conditional) constant propagation analyses.
Depends on D127064
Reviewed By: rriddle, phisiart
Differential Revision: https://reviews.llvm.org/D127139
This patch implements the analysis state classes needed for sparse data-flow analysis and implements a dead-code analysis using those states to determine liveness of blocks, control-flow edges, region predecessors, and function callsites.
Depends on D126751
Reviewed By: rriddle, phisiart
Differential Revision: https://reviews.llvm.org/D127064
