From bde21b624585255dbd28c0bc0e93d37045b5a9a9 Mon Sep 17 00:00:00 2001 From: Chris Lattner Date: Fri, 28 May 2021 15:12:44 -0700 Subject: [PATCH] [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 --- mlir/include/mlir/IR/OpDefinition.h | 12 +++---- mlir/include/mlir/IR/Region.h | 18 +++++----- mlir/lib/IR/Operation.cpp | 48 +++++++++++++++++++++++++ mlir/lib/IR/Region.cpp | 54 ----------------------------- 4 files changed, 61 insertions(+), 71 deletions(-) diff --git a/mlir/include/mlir/IR/OpDefinition.h b/mlir/include/mlir/IR/OpDefinition.h index b2e50324f75d..dadf028d281f 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 { 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 @@ private: llvm::is_detected; /// Trait to check if T provides a general 'fold' method. template - using has_fold = decltype( - std::declval().fold(std::declval>(), - std::declval &>())); + using has_fold = decltype(std::declval().fold( + std::declval>(), + std::declval &>())); template using detect_has_fold = llvm::is_detected; /// 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 bc35e2d231a5..17a5072faa11 100644 --- a/mlir/include/mlir/IR/Region.h +++ b/mlir/include/mlir/IR/Region.h @@ -164,13 +164,16 @@ public: /// Return iterators that walk operations of type 'T' nested directly within /// this region. - template op_iterator op_begin() { + template + op_iterator op_begin() { return detail::op_filter_iterator(op_begin(), op_end()); } - template op_iterator op_end() { + template + op_iterator op_end() { return detail::op_filter_iterator(op_end(), op_end()); } - template iterator_range> getOps() { + template + iterator_range> getOps() { auto endIt = op_end(); return {detail::op_filter_iterator(op_begin(), endIt), detail::op_filter_iterator(endIt, endIt)}; @@ -189,7 +192,8 @@ public: /// Find the first parent operation of the given type, or nullptr if there is /// no ancestor operation. - template ParentT getParentOfType() { + template + ParentT getParentOfType() { auto *region = this; do { if (auto parent = dyn_cast_or_null(region->container)) @@ -226,12 +230,6 @@ public: 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 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 45afaafd5934..74d8b4450e3e 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() && + "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 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()) { + for (Region &subRegion : op.getRegions()) + pendingRegions.push_back(&subRegion); + } + } + } + } + + return success(); +} + bool OpTrait::hasElementwiseMappableTraits(Operation *op) { return op->hasTrait() && op->hasTrait() && op->hasTrait() && op->hasTrait(); diff --git a/mlir/lib/IR/Region.cpp b/mlir/lib/IR/Region.cpp index 76101382cd58..a06b89fc3ffa 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 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 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 noteLoc) { - return isIsolatedAbove(*this, *this, noteLoc); -} - Region *llvm::ilist_traits<::mlir::Block>::getParentRegion() { size_t Offset( size_t(&((Region *)nullptr->*Region::getSublistAccess(nullptr)))); -- GitLab