[Mlir-commits] [mlir] bde21b6 - [Verifier] Significantly speed up IsolatedFromAbove checking. NFC.
Chris Lattner
llvmlistbot at llvm.org
Fri May 28 16:13:51 PDT 2021
Author: Chris Lattner
Date: 2021-05-28T16:13:45-07:00
New Revision: bde21b624585255dbd28c0bc0e93d37045b5a9a9
URL: https://github.com/llvm/llvm-project/commit/bde21b624585255dbd28c0bc0e93d37045b5a9a9
DIFF: https://github.com/llvm/llvm-project/commit/bde21b624585255dbd28c0bc0e93d37045b5a9a9.diff
LOG: [Verifier] Significantly speed up IsolatedFromAbove checking. NFC.
The implementation had a couple of problems, including checking
"isProperAncestor" in an inefficient way. It also recursed into
other "isolated from above" ops. In the case of CIRCT, we get
three levels of isolated ops:
mlir::ModuleOp
firrtl::CircuitOp
firrtl::FModuleOp
The verification for module would recurse into the circuits and
fmodules checking them. The verifier hook for circuit would
recurse into all the modules reverifying them, fmoduleop would
then reverify them. The same happens for mlir::ModuleOp and Func.
While here, fix an old design problem: IsolatedFromAbove checking
was implemented by a method on the Region class, which isn't
actually general and isn't used by anything else. Move it over
to be a trait impl verifier method like the others and specialize
it for its task.
Differential Revision: https://reviews.llvm.org/D103345
Added:
Modified:
mlir/include/mlir/IR/OpDefinition.h
mlir/include/mlir/IR/Region.h
mlir/lib/IR/Operation.cpp
mlir/lib/IR/Region.cpp
Removed:
################################################################################
diff --git a/mlir/include/mlir/IR/OpDefinition.h b/mlir/include/mlir/IR/OpDefinition.h
index b2e50324f75df..dadf028d281f8 100644
--- a/mlir/include/mlir/IR/OpDefinition.h
+++ b/mlir/include/mlir/IR/OpDefinition.h
@@ -271,6 +271,7 @@ LogicalResult verifyOperandSizeAttr(Operation *op, StringRef sizeAttrName);
LogicalResult verifyResultSizeAttr(Operation *op, StringRef sizeAttrName);
LogicalResult verifyNoRegionArguments(Operation *op);
LogicalResult verifyElementwise(Operation *op);
+LogicalResult verifyIsIsolatedFromAbove(Operation *op);
} // namespace impl
/// Helper class for implementing traits. Clients are not expected to interact
@@ -1163,10 +1164,7 @@ class IsIsolatedFromAbove
: public TraitBase<ConcreteType, IsIsolatedFromAbove> {
public:
static LogicalResult verifyTrait(Operation *op) {
- for (auto ®ion : op->getRegions())
- if (!region.isIsolatedFromAbove(op->getLoc()))
- return failure();
- return success();
+ return impl::verifyIsIsolatedFromAbove(op);
}
};
@@ -1631,9 +1629,9 @@ class Op : public OpState, public Traits<ConcreteType>... {
llvm::is_detected<has_single_result_fold, T>;
/// Trait to check if T provides a general 'fold' method.
template <typename T, typename... Args>
- using has_fold = decltype(
- std::declval<T>().fold(std::declval<ArrayRef<Attribute>>(),
- std::declval<SmallVectorImpl<OpFoldResult> &>()));
+ using has_fold = decltype(std::declval<T>().fold(
+ std::declval<ArrayRef<Attribute>>(),
+ std::declval<SmallVectorImpl<OpFoldResult> &>()));
template <typename T>
using detect_has_fold = llvm::is_detected<has_fold, T>;
/// Trait to check if T provides a 'print' method.
diff --git a/mlir/include/mlir/IR/Region.h b/mlir/include/mlir/IR/Region.h
index bc35e2d231a50..17a5072faa11c 100644
--- a/mlir/include/mlir/IR/Region.h
+++ b/mlir/include/mlir/IR/Region.h
@@ -164,13 +164,16 @@ class Region {
/// Return iterators that walk operations of type 'T' nested directly within
/// this region.
- template <typename OpT> op_iterator<OpT> op_begin() {
+ template <typename OpT>
+ op_iterator<OpT> op_begin() {
return detail::op_filter_iterator<OpT, OpIterator>(op_begin(), op_end());
}
- template <typename OpT> op_iterator<OpT> op_end() {
+ template <typename OpT>
+ op_iterator<OpT> op_end() {
return detail::op_filter_iterator<OpT, OpIterator>(op_end(), op_end());
}
- template <typename OpT> iterator_range<op_iterator<OpT>> getOps() {
+ template <typename OpT>
+ iterator_range<op_iterator<OpT>> getOps() {
auto endIt = op_end();
return {detail::op_filter_iterator<OpT, OpIterator>(op_begin(), endIt),
detail::op_filter_iterator<OpT, OpIterator>(endIt, endIt)};
@@ -189,7 +192,8 @@ class Region {
/// Find the first parent operation of the given type, or nullptr if there is
/// no ancestor operation.
- template <typename ParentT> ParentT getParentOfType() {
+ template <typename ParentT>
+ ParentT getParentOfType() {
auto *region = this;
do {
if (auto parent = dyn_cast_or_null<ParentT>(region->container))
@@ -226,12 +230,6 @@ class Region {
blocks.splice(blocks.end(), other.getBlocks());
}
- /// Check that this does not use any value defined outside it.
- /// Emit errors if `noteLoc` is provided; this location is used to point
- /// to the operation containing the region, the actual error is reported at
- /// the operation with an offending use.
- bool isIsolatedFromAbove(Optional<Location> noteLoc = llvm::None);
-
/// Returns 'block' if 'block' lies in this region, or otherwise finds the
/// ancestor of 'block' that lies in this region. Returns nullptr if the
/// latter fails.
diff --git a/mlir/lib/IR/Operation.cpp b/mlir/lib/IR/Operation.cpp
index 45afaafd5934f..74d8b4450e3e3 100644
--- a/mlir/lib/IR/Operation.cpp
+++ b/mlir/lib/IR/Operation.cpp
@@ -1104,6 +1104,54 @@ LogicalResult OpTrait::impl::verifyElementwise(Operation *op) {
return success();
}
+/// Check for any values used by operations regions attached to the
+/// specified "IsIsolatedFromAbove" operation defined outside of it.
+LogicalResult OpTrait::impl::verifyIsIsolatedFromAbove(Operation *isolatedOp) {
+ assert(isolatedOp->hasTrait<OpTrait::IsIsolatedFromAbove>() &&
+ "Intended to check IsolatedFromAbove ops");
+
+ // List of regions to analyze. Each region is processed independently, with
+ // respect to the common `limit` region, so we can look at them in any order.
+ // Therefore, use a simple vector and push/pop back the current region.
+ SmallVector<Region *, 8> pendingRegions;
+ for (auto ®ion : isolatedOp->getRegions()) {
+ pendingRegions.push_back(®ion);
+
+ // Traverse all operations in the region.
+ while (!pendingRegions.empty()) {
+ for (Operation &op : pendingRegions.pop_back_val()->getOps()) {
+ for (Value operand : op.getOperands()) {
+ // operand should be non-null here if the IR is well-formed. But
+ // we don't assert here as this function is called from the verifier
+ // and so could be called on invalid IR.
+ if (!operand)
+ return op.emitOpError("operation's operand is null");
+
+ // Check that any value that is used by an operation is defined in the
+ // same region as either an operation result.
+ auto *operandRegion = operand.getParentRegion();
+ if (!region.isAncestor(operandRegion)) {
+ return op.emitOpError("using value defined outside the region")
+ .attachNote(isolatedOp->getLoc())
+ << "required by region isolation constraints";
+ }
+ }
+
+ // Schedule any regions in the operation for further checking. Don't
+ // recurse into other IsolatedFromAbove ops, because they will check
+ // themselves.
+ if (op.getNumRegions() &&
+ !op.hasTrait<OpTrait::IsIsolatedFromAbove>()) {
+ for (Region &subRegion : op.getRegions())
+ pendingRegions.push_back(&subRegion);
+ }
+ }
+ }
+ }
+
+ return success();
+}
+
bool OpTrait::hasElementwiseMappableTraits(Operation *op) {
return op->hasTrait<Elementwise>() && op->hasTrait<Scalarizable>() &&
op->hasTrait<Vectorizable>() && op->hasTrait<Tensorizable>();
diff --git a/mlir/lib/IR/Region.cpp b/mlir/lib/IR/Region.cpp
index 76101382cd585..a06b89fc3ffa6 100644
--- a/mlir/lib/IR/Region.cpp
+++ b/mlir/lib/IR/Region.cpp
@@ -137,60 +137,6 @@ void Region::dropAllReferences() {
b.dropAllReferences();
}
-/// Check if there are any values used by operations in `region` defined
-/// outside its ancestor region `limit`. That is, given `A{B{C{}}}` with region
-/// `C` and limit `B`, the values defined in `B` can be used but the values
-/// defined in `A` cannot. Emit errors if `noteLoc` is provided; this location
-/// is used to point to the operation containing the region, the actual error is
-/// reported at the operation with an offending use.
-static bool isIsolatedAbove(Region ®ion, Region &limit,
- Optional<Location> noteLoc) {
- assert(limit.isAncestor(®ion) &&
- "expected isolation limit to be an ancestor of the given region");
-
- // List of regions to analyze. Each region is processed independently, with
- // respect to the common `limit` region, so we can look at them in any order.
- // Therefore, use a simple vector and push/pop back the current region.
- SmallVector<Region *, 8> pendingRegions;
- pendingRegions.push_back(®ion);
-
- // Traverse all operations in the region.
- while (!pendingRegions.empty()) {
- for (Operation &op : pendingRegions.pop_back_val()->getOps()) {
- for (Value operand : op.getOperands()) {
- // operand should be non-null here if the IR is well-formed. But
- // we don't assert here as this function is called from the verifier
- // and so could be called on invalid IR.
- if (!operand) {
- if (noteLoc)
- op.emitOpError("block's operand not defined").attachNote(noteLoc);
- return false;
- }
-
- // Check that any value that is used by an operation is defined in the
- // same region as either an operation result or a block argument.
- if (operand.getParentRegion()->isProperAncestor(&limit)) {
- if (noteLoc) {
- op.emitOpError("using value defined outside the region")
- .attachNote(noteLoc)
- << "required by region isolation constraints";
- }
- return false;
- }
- }
- // Schedule any regions the operations contain for further checking.
- pendingRegions.reserve(pendingRegions.size() + op.getNumRegions());
- for (Region &subRegion : op.getRegions())
- pendingRegions.push_back(&subRegion);
- }
- }
- return true;
-}
-
-bool Region::isIsolatedFromAbove(Optional<Location> noteLoc) {
- return isIsolatedAbove(*this, *this, noteLoc);
-}
-
Region *llvm::ilist_traits<::mlir::Block>::getParentRegion() {
size_t Offset(
size_t(&((Region *)nullptr->*Region::getSublistAccess(nullptr))));
More information about the Mlir-commits
mailing list