diff options
23 files changed, 180 insertions, 107 deletions
diff --git a/mlir/docs/Traits.md b/mlir/docs/Traits.md index e825eb6..9877d90 100644 --- a/mlir/docs/Traits.md +++ b/mlir/docs/Traits.md @@ -208,12 +208,6 @@ foo.region_op { This trait is an important structural property of the IR, and enables operations to have [passes](WritingAPass.md) scheduled under them. -### NoSideEffect - -* `OpTrait::HasNoSideEffect` -- `NoSideEffect` - -This trait signifies that the operation is pure and has no visible side effects. - ### Single Block with Implicit Terminator * `OpTrait::SingleBlockImplicitTerminator<typename TerminatorOpType>` : diff --git a/mlir/docs/Tutorials/Toy/Ch-2.md b/mlir/docs/Tutorials/Toy/Ch-2.md index 881e077..c8be481 100755 --- a/mlir/docs/Tutorials/Toy/Ch-2.md +++ b/mlir/docs/Tutorials/Toy/Ch-2.md @@ -206,9 +206,7 @@ class ConstantOp : public mlir::Op<ConstantOp, /// The ConstantOp takes no inputs. mlir::OpTrait::ZeroOperands, /// The ConstantOp returns a single result. - mlir::OpTrait::OneResult, - /// The ConstantOp is pure and has no visible side-effects. - mlir::OpTrait::HasNoSideEffect> { + mlir::OpTrait::OneResult> { public: /// Inherit the constructors from the base Op class. @@ -335,15 +333,13 @@ operation. We define a toy operation by inheriting from our base 'Toy_Op' class above. Here we provide the mnemonic and a list of traits for the operation. The [mnemonic](../../OpDefinitions.md#operation-name) here matches the one given in -`ConstantOp::getOperationName` without the dialect prefix; `toy.`. The constant -operation here is also marked as 'NoSideEffect'. This is an ODS trait, and -matches one-to-one with the trait we providing when defining `ConstantOp`: -`mlir::OpTrait::HasNoSideEffect`. Missing here from our C++ definition are the -`ZeroOperands` and `OneResult` traits; these will be automatically inferred -based upon the `arguments` and `results` fields we define later. +`ConstantOp::getOperationName` without the dialect prefix; `toy.`. Missing here +from our C++ definition are the `ZeroOperands` and `OneResult` traits; these +will be automatically inferred based upon the `arguments` and `results` fields +we define later. ```tablegen -def ConstantOp : Toy_Op<"constant", [NoSideEffect]> { +def ConstantOp : Toy_Op<"constant"> { } ``` @@ -369,7 +365,7 @@ values. The results correspond to a set of types for the values produced by the operation: ```tablegen -def ConstantOp : Toy_Op<"constant", [NoSideEffect]> { +def ConstantOp : Toy_Op<"constant"> { // The constant operation takes an attribute as the only input. // `F64ElementsAttr` corresponds to a 64-bit floating-point ElementsAttr. let arguments = (ins F64ElementsAttr:$value); @@ -394,7 +390,7 @@ for users of the dialect and can even be used to auto-generate Markdown documents. ```tablegen -def ConstantOp : Toy_Op<"constant", [NoSideEffect]> { +def ConstantOp : Toy_Op<"constant"> { // Provide a summary and description for this operation. This can be used to // auto-generate documentation of the operations within our dialect. let summary = "constant operation"; @@ -432,7 +428,7 @@ as part of `ConstantOp::verify`. This blob can assume that all of the other invariants of the operation have already been verified: ```tablegen -def ConstantOp : Toy_Op<"constant", [NoSideEffect]> { +def ConstantOp : Toy_Op<"constant"> { // Provide a summary and description for this operation. This can be used to // auto-generate documentation of the operations within our dialect. let summary = "constant operation"; @@ -472,7 +468,7 @@ of C++ parameters, as well as an optional code block that can be used to specify the implementation inline. ```tablegen -def ConstantOp : Toy_Op<"constant", [NoSideEffect]> { +def ConstantOp : Toy_Op<"constant"> { ... // Add custom build methods for the constant operation. These methods populate diff --git a/mlir/include/mlir/IR/Operation.h b/mlir/include/mlir/IR/Operation.h index 781dbcd..4183845 100644 --- a/mlir/include/mlir/IR/Operation.h +++ b/mlir/include/mlir/IR/Operation.h @@ -407,13 +407,6 @@ public: return false; } - /// Returns whether the operation has side-effects. - bool hasNoSideEffect() { - if (auto *absOp = getAbstractOperation()) - return absOp->hasProperty(OperationProperty::NoSideEffect); - return false; - } - /// Represents the status of whether an operation is a terminator. We /// represent an 'unknown' status because we want to support unregistered /// terminators. diff --git a/mlir/include/mlir/IR/OperationSupport.h b/mlir/include/mlir/IR/OperationSupport.h index 2d4a968..8928886 100644 --- a/mlir/include/mlir/IR/OperationSupport.h +++ b/mlir/include/mlir/IR/OperationSupport.h @@ -68,19 +68,15 @@ enum class OperationProperty { /// results. Commutative = 0x1, - /// This bit is set for operations that have no side effects: that means that - /// they do not read or write memory, or access any hidden state. - NoSideEffect = 0x2, - /// This bit is set for an operation if it is a terminator: that means /// an operation at the end of a block. - Terminator = 0x4, + Terminator = 0x2, /// This bit is set for operations that are completely isolated from above. /// This is used for operations whose regions are explicit capture only, i.e. /// they are never allowed to implicitly reference values defined above the /// parent operation. - IsolatedFromAbove = 0x8, + IsolatedFromAbove = 0x4, }; /// This is a "type erased" representation of a registered operation. This diff --git a/mlir/include/mlir/Interfaces/SideEffects.h b/mlir/include/mlir/Interfaces/SideEffects.h index 96714da..e0fd175 100644 --- a/mlir/include/mlir/Interfaces/SideEffects.h +++ b/mlir/include/mlir/Interfaces/SideEffects.h @@ -163,15 +163,6 @@ private: //===----------------------------------------------------------------------===// namespace OpTrait { -/// This trait indicates that an operation never has side effects. -template <typename ConcreteType> -class HasNoSideEffect : public TraitBase<ConcreteType, HasNoSideEffect> { -public: - static AbstractOperation::OperationProperties getTraitProperties() { - return static_cast<AbstractOperation::OperationProperties>( - OperationProperty::NoSideEffect); - } -}; /// This trait indicates that the side effects of an operation includes the /// effects of operations nested within its regions. If the operation has no /// derived effects interfaces, the operation itself can be assumed to have no @@ -221,10 +212,24 @@ struct Write : public Effect::Base<Write> {}; //===----------------------------------------------------------------------===// // SideEffect Interfaces +//===----------------------------------------------------------------------===// /// Include the definitions of the side effect interfaces. #include "mlir/Interfaces/SideEffectInterfaces.h.inc" +//===----------------------------------------------------------------------===// +// SideEffect Utilities +//===----------------------------------------------------------------------===// + +/// Return true if the given operation is unused, and has no side effects on +/// memory that prevent erasing. +bool isOpTriviallyDead(Operation *op); + +/// Return true if the given operation would be dead if unused, and has no side +/// effects on memory that would prevent erasing. This is equivalent to checking +/// `isOpTriviallyDead` if `op` was unused. +bool wouldOpBeTriviallyDead(Operation *op); + } // end namespace mlir #endif // MLIR_INTERFACES_SIDEEFFECTS_H diff --git a/mlir/include/mlir/Interfaces/SideEffects.td b/mlir/include/mlir/Interfaces/SideEffects.td index ebf3da6..8bb4072 100644 --- a/mlir/include/mlir/Interfaces/SideEffects.td +++ b/mlir/include/mlir/Interfaces/SideEffects.td @@ -98,6 +98,13 @@ class EffectOpInterfaceBase<string name, string baseEffect> getEffects(effects); return effects.empty(); } + + /// Returns if the given operation has no effects for this interface. + static bool hasNoEffect(Operation *op) { + if (auto interface = dyn_cast<}] # name # [{>(op)) + return interface.hasNoEffect(); + return op->hasTrait<OpTrait::HasRecursiveSideEffects>(); + } }]; // The base effect name of this interface. @@ -108,11 +115,8 @@ class EffectOpInterfaceBase<string name, string baseEffect> // effect interfaces to define their effects. class SideEffect<EffectOpInterfaceBase interface, string effectName, string resourceName> : OpVariableDecorator { - /// The parent interface that the effect belongs to. - string interfaceTrait = interface.trait; - /// The name of the base effects class. - string baseEffect = interface.baseEffectName; + string baseEffectName = interface.baseEffectName; /// The derived effect that is being applied. string effect = effectName; @@ -128,6 +132,9 @@ class SideEffectsTraitBase<EffectOpInterfaceBase parentInterface, /// The name of the interface trait to use. let trait = parentInterface.trait; + /// The name of the base effects class. + string baseEffectName = parentInterface.baseEffectName; + /// The derived effects being applied. list<SideEffect> effects = staticEffects; } @@ -193,7 +200,7 @@ def MemWrite : MemWrite<"">; //===----------------------------------------------------------------------===// // Op has no side effect. -def NoSideEffect : NativeOpTrait<"HasNoSideEffect">; +def NoSideEffect : MemoryEffects<[]>; // Op has recursively computed side effects. def RecursiveSideEffects : NativeOpTrait<"HasRecursiveSideEffects">; diff --git a/mlir/include/mlir/TableGen/SideEffects.h b/mlir/include/mlir/TableGen/SideEffects.h index c93502c..eb2a068 100644 --- a/mlir/include/mlir/TableGen/SideEffects.h +++ b/mlir/include/mlir/TableGen/SideEffects.h @@ -27,10 +27,7 @@ public: StringRef getName() const; // Return the name of the base C++ effect. - StringRef getBaseName() const; - - // Return the name of the parent interface trait. - StringRef getInterfaceTrait() const; + StringRef getBaseEffectName() const; // Return the name of the resource class. StringRef getResource() const; @@ -46,6 +43,9 @@ public: // Return the effects that are attached to the side effect interface. Operator::var_decorator_range getEffects() const; + // Return the name of the base C++ effect. + StringRef getBaseEffectName() const; + static bool classof(const OpTrait *t); }; diff --git a/mlir/include/mlir/Transforms/FoldUtils.h b/mlir/include/mlir/Transforms/FoldUtils.h index 56a78b1..83ce3bf 100644 --- a/mlir/include/mlir/Transforms/FoldUtils.h +++ b/mlir/include/mlir/Transforms/FoldUtils.h @@ -106,6 +106,9 @@ public: return op; } + /// Clear out any constants cached inside of the folder. + void clear(); + private: /// This map keeps track of uniqued constants by dialect, attribute, and type. /// A constant operation materializes an attribute with a type. Dialects may diff --git a/mlir/lib/Analysis/Utils.cpp b/mlir/lib/Analysis/Utils.cpp index 14635a1..7b3cd58 100644 --- a/mlir/lib/Analysis/Utils.cpp +++ b/mlir/lib/Analysis/Utils.cpp @@ -974,11 +974,12 @@ void mlir::getSequentialLoops(AffineForOp forOp, bool mlir::isLoopParallel(AffineForOp forOp) { // Collect all load and store ops in loop nest rooted at 'forOp'. SmallVector<Operation *, 8> loadAndStoreOpInsts; - auto walkResult = forOp.walk([&](Operation *opInst) { + auto walkResult = forOp.walk([&](Operation *opInst) -> WalkResult { if (isa<AffineLoadOp>(opInst) || isa<AffineStoreOp>(opInst)) loadAndStoreOpInsts.push_back(opInst); else if (!isa<AffineForOp>(opInst) && !isa<AffineTerminatorOp>(opInst) && - !isa<AffineIfOp>(opInst) && !opInst->hasNoSideEffect()) + !isa<AffineIfOp>(opInst) && + !MemoryEffectOpInterface::hasNoEffect(opInst)) return WalkResult::interrupt(); return WalkResult::advance(); diff --git a/mlir/lib/Conversion/LoopsToGPU/LoopsToGPU.cpp b/mlir/lib/Conversion/LoopsToGPU/LoopsToGPU.cpp index 293d935..9ba3f40 100644 --- a/mlir/lib/Conversion/LoopsToGPU/LoopsToGPU.cpp +++ b/mlir/lib/Conversion/LoopsToGPU/LoopsToGPU.cpp @@ -797,7 +797,9 @@ ParallelToGpuLaunchLowering::matchAndRewrite(ParallelOp parallelOp, Operation *clone = rewriter.clone(*op, cloningMap); cloningMap.map(op->getResults(), clone->getResults()); // Check for side effects. - seenSideeffects |= !clone->hasNoSideEffect(); + // TODO: Handle region side effects properly. + seenSideeffects |= !MemoryEffectOpInterface::hasNoEffect(clone) || + clone->getNumRegions() != 0; // If we are no longer in the innermost scope, sideeffects are disallowed. if (seenSideeffects && leftNestingScope) return matchFailure(); diff --git a/mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp b/mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp index de29848..3274abd 100644 --- a/mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp +++ b/mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp @@ -491,9 +491,7 @@ static void tileLinalgOps(FuncOp f, ArrayRef<int64_t> tileSizes) { op.erase(); }); f.walk([](LinalgOp op) { - if (!op.getOperation()->hasNoSideEffect()) - return; - if (op.getOperation()->use_empty()) + if (isOpTriviallyDead(op)) op.erase(); }); } diff --git a/mlir/lib/Dialect/LoopOps/Transforms/ParallelLoopFusion.cpp b/mlir/lib/Dialect/LoopOps/Transforms/ParallelLoopFusion.cpp index 98f8313..b84cfa5 100644 --- a/mlir/lib/Dialect/LoopOps/Transforms/ParallelLoopFusion.cpp +++ b/mlir/lib/Dialect/LoopOps/Transforms/ParallelLoopFusion.cpp @@ -132,13 +132,13 @@ static void fuseIfLegal(ParallelOp firstPloop, ParallelOp secondPloop, void mlir::loop::naivelyFuseParallelOps(Region ®ion) { OpBuilder b(region); // Consider every single block and attempt to fuse adjacent loops. - for (auto &block : region.getBlocks()) { + for (auto &block : region) { SmallVector<SmallVector<ParallelOp, 8>, 1> ploopChains{{}}; // Not using `walk()` to traverse only top-level parallel loops and also // make sure that there are no side-effecting ops between the parallel // loops. bool noSideEffects = true; - for (auto &op : block.getOperations()) { + for (auto &op : block) { if (auto ploop = dyn_cast<ParallelOp>(op)) { if (noSideEffects) { ploopChains.back().push_back(ploop); @@ -148,7 +148,9 @@ void mlir::loop::naivelyFuseParallelOps(Region ®ion) { } continue; } - noSideEffects &= op.hasNoSideEffect(); + // TODO: Handle region side effects properly. + noSideEffects &= + MemoryEffectOpInterface::hasNoEffect(&op) && op.getNumRegions() == 0; } for (ArrayRef<ParallelOp> ploops : ploopChains) { for (int i = 0, e = ploops.size(); i + 1 < e; ++i) diff --git a/mlir/lib/Interfaces/SideEffects.cpp b/mlir/lib/Interfaces/SideEffects.cpp index da43239..53406c6 100644 --- a/mlir/lib/Interfaces/SideEffects.cpp +++ b/mlir/lib/Interfaces/SideEffects.cpp @@ -25,3 +25,70 @@ bool MemoryEffects::Effect::classof(const SideEffects::Effect *effect) { return isa<Allocate>(effect) || isa<Free>(effect) || isa<Read>(effect) || isa<Write>(effect); } + +//===----------------------------------------------------------------------===// +// SideEffect Utilities +//===----------------------------------------------------------------------===// + +bool mlir::isOpTriviallyDead(Operation *op) { + return op->use_empty() && wouldOpBeTriviallyDead(op); +} + +/// Internal implementation of `mlir::wouldOpBeTriviallyDead` that also +/// considers terminator operations as dead if they have no side effects. This +/// allows for marking region operations as trivially dead without always being +/// conservative of terminators. +static bool wouldOpBeTriviallyDeadImpl(Operation *rootOp) { + // The set of operations to consider when checking for side effects. + SmallVector<Operation *, 1> effectingOps(1, rootOp); + while (!effectingOps.empty()) { + Operation *op = effectingOps.pop_back_val(); + + // If the operation has recursive effects, push all of the nested operations + // on to the stack to consider. + bool hasRecursiveEffects = op->hasTrait<OpTrait::HasRecursiveSideEffects>(); + if (hasRecursiveEffects) { + for (Region ®ion : op->getRegions()) { + for (auto &block : region) { + for (auto &nestedOp : block) + effectingOps.push_back(&nestedOp); + } + } + } + + // If the op has memory effects, try to characterize them to see if the op + // is trivially dead here. + if (auto effectInterface = dyn_cast<MemoryEffectOpInterface>(op)) { + // Check to see if this op either has no effects, or only allocates/reads + // memory. + SmallVector<MemoryEffects::EffectInstance, 1> effects; + effectInterface.getEffects(effects); + if (!llvm::all_of(effects, [](const auto &it) { + return isa<MemoryEffects::Read>(it.getEffect()) || + isa<MemoryEffects::Allocate>(it.getEffect()); + })) { + return false; + } + continue; + + // Otherwise, if the op has recursive side effects we can treat the + // operation itself as having no effects. + } else if (hasRecursiveEffects) { + continue; + } + + // If there were no effect interfaces, we treat this op as conservatively + // having effects. + return false; + } + + // If we get here, none of the operations had effects that prevented marking + // 'op' as dead. + return true; +} + +bool mlir::wouldOpBeTriviallyDead(Operation *op) { + if (!op->isKnownNonTerminator()) + return false; + return wouldOpBeTriviallyDeadImpl(op); +} diff --git a/mlir/lib/TableGen/SideEffects.cpp b/mlir/lib/TableGen/SideEffects.cpp index 0b334b8..7fbeffa 100644 --- a/mlir/lib/TableGen/SideEffects.cpp +++ b/mlir/lib/TableGen/SideEffects.cpp @@ -20,12 +20,8 @@ StringRef SideEffect::getName() const { return def->getValueAsString("effect"); } -StringRef SideEffect::getBaseName() const { - return def->getValueAsString("baseEffect"); -} - -StringRef SideEffect::getInterfaceTrait() const { - return def->getValueAsString("interfaceTrait"); +StringRef SideEffect::getBaseEffectName() const { + return def->getValueAsString("baseEffectName"); } StringRef SideEffect::getResource() const { @@ -46,6 +42,10 @@ Operator::var_decorator_range SideEffectTrait::getEffects() const { return {listInit->begin(), listInit->end()}; } +StringRef SideEffectTrait::getBaseEffectName() const { + return def->getValueAsString("baseEffectName"); +} + bool SideEffectTrait::classof(const OpTrait *t) { return t->getDef().isSubClassOf("SideEffectsTraitBase"); } diff --git a/mlir/lib/Transforms/CSE.cpp b/mlir/lib/Transforms/CSE.cpp index 3a76594..42ba715 100644 --- a/mlir/lib/Transforms/CSE.cpp +++ b/mlir/lib/Transforms/CSE.cpp @@ -127,6 +127,13 @@ LogicalResult CSE::simplifyOperation(ScopedMapTy &knownValues, Operation *op) { if (op->isKnownTerminator()) return failure(); + // If the operation is already trivially dead just add it to the erase list. + if (isOpTriviallyDead(op)) { + opsToErase.push_back(op); + ++numDCE; + return success(); + } + // Don't simplify operations with nested blocks. We don't currently model // equality comparisons correctly among other things. It is also unclear // whether we would want to CSE such operations. @@ -135,16 +142,9 @@ LogicalResult CSE::simplifyOperation(ScopedMapTy &knownValues, Operation *op) { // TODO(riverriddle) We currently only eliminate non side-effecting // operations. - if (!op->hasNoSideEffect()) + if (!MemoryEffectOpInterface::hasNoEffect(op)) return failure(); - // If the operation is already trivially dead just add it to the erase list. - if (op->use_empty()) { - opsToErase.push_back(op); - ++numDCE; - return success(); - } - // Look for an existing definition for the operation. if (auto *existing = knownValues.lookup(op)) { // If we find one then replace all uses of the current operation with the diff --git a/mlir/lib/Transforms/LoopInvariantCodeMotion.cpp b/mlir/lib/Transforms/LoopInvariantCodeMotion.cpp index a452e33..7300948 100644 --- a/mlir/lib/Transforms/LoopInvariantCodeMotion.cpp +++ b/mlir/lib/Transforms/LoopInvariantCodeMotion.cpp @@ -49,16 +49,18 @@ static bool canBeHoisted(Operation *op, if (auto memInterface = dyn_cast<MemoryEffectOpInterface>(op)) { if (!memInterface.hasNoEffect()) return false; - } else if (!op->hasNoSideEffect() && - !op->hasTrait<OpTrait::HasRecursiveSideEffects>()) { + // If the operation doesn't have side effects and it doesn't recursively + // have side effects, it can always be hoisted. + if (!op->hasTrait<OpTrait::HasRecursiveSideEffects>()) + return true; + + // Otherwise, if the operation doesn't provide the memory effect interface + // and it doesn't have recursive side effects we treat it conservatively as + // side-effecting. + } else if (!op->hasTrait<OpTrait::HasRecursiveSideEffects>()) { return false; } - // If the operation doesn't have side effects and it doesn't recursively - // have side effects, it can always be hoisted. - if (!op->hasTrait<OpTrait::HasRecursiveSideEffects>()) - return true; - // Recurse into the regions for this op and check whether the contained ops // can be hoisted. for (auto ®ion : op->getRegions()) { diff --git a/mlir/lib/Transforms/Utils/FoldUtils.cpp b/mlir/lib/Transforms/Utils/FoldUtils.cpp index f374d38..66535ec 100644 --- a/mlir/lib/Transforms/Utils/FoldUtils.cpp +++ b/mlir/lib/Transforms/Utils/FoldUtils.cpp @@ -126,6 +126,12 @@ void OperationFolder::notifyRemoval(Operation *op) { referencedDialects.erase(it); } +/// Clear out any constants cached inside of the folder. +void OperationFolder::clear() { + foldScopes.clear(); + referencedDialects.clear(); +} + /// Tries to perform folding on the given `op`. If successful, populates /// `results` with the results of the folding. LogicalResult OperationFolder::tryToFold( diff --git a/mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp b/mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp index f9a9be5..e40a4d9 100644 --- a/mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp +++ b/mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp @@ -10,9 +10,8 @@ // //===----------------------------------------------------------------------===// -#include "mlir/Dialect/StandardOps/IR/Ops.h" -#include "mlir/IR/Builders.h" #include "mlir/IR/PatternMatch.h" +#include "mlir/Interfaces/SideEffects.h" #include "mlir/Transforms/FoldUtils.h" #include "mlir/Transforms/RegionUtils.h" #include "llvm/ADT/DenseMap.h" @@ -162,11 +161,8 @@ bool GreedyPatternRewriteDriver::simplify(MutableArrayRef<Region> regions, if (op == nullptr) continue; - // If the operation has no side effects, and no users, then it is - // trivially dead - remove it. - if (op->isKnownNonTerminator() && op->hasNoSideEffect() && - op->use_empty()) { - // Be careful to update bookkeeping. + // If the operation is trivially dead - remove it. + if (isOpTriviallyDead(op)) { notifyOperationRemoved(op); op->erase(); continue; @@ -204,7 +200,10 @@ bool GreedyPatternRewriteDriver::simplify(MutableArrayRef<Region> regions, // After applying patterns, make sure that the CFG of each of the regions is // kept up to date. - changed |= succeeded(simplifyRegions(regions)); + if (succeeded(simplifyRegions(regions))) { + folder.clear(); + changed = true; + } } while (changed && ++i < maxIterations); // Whether the rewrite converges, i.e. wasn't changed in the last iteration. return !changed; diff --git a/mlir/lib/Transforms/Utils/LoopUtils.cpp b/mlir/lib/Transforms/Utils/LoopUtils.cpp index a02c71a..7095e55 100644 --- a/mlir/lib/Transforms/Utils/LoopUtils.cpp +++ b/mlir/lib/Transforms/Utils/LoopUtils.cpp @@ -887,7 +887,7 @@ static LogicalResult hoistOpsBetween(loop::ForOp outer, loop::ForOp inner) { } // Skip if op has side effects. // TODO(ntv): loads to immutable memory regions are ok. - if (!op.hasNoSideEffect()) { + if (!MemoryEffectOpInterface::hasNoEffect(&op)) { status = failure(); continue; } diff --git a/mlir/lib/Transforms/Utils/RegionUtils.cpp b/mlir/lib/Transforms/Utils/RegionUtils.cpp index 78bb609b..162091c 100644 --- a/mlir/lib/Transforms/Utils/RegionUtils.cpp +++ b/mlir/lib/Transforms/Utils/RegionUtils.cpp @@ -12,6 +12,7 @@ #include "mlir/IR/RegionGraphTraits.h" #include "mlir/IR/Value.h" #include "mlir/Interfaces/ControlFlowInterfaces.h" +#include "mlir/Interfaces/SideEffects.h" #include "llvm/ADT/DepthFirstIterator.h" #include "llvm/ADT/PostOrderIterator.h" @@ -196,9 +197,8 @@ static bool isOpIntrinsicallyLive(Operation *op) { if (!op->isKnownNonTerminator()) return true; // If the op has a side effect, we treat it as live. - if (!op->hasNoSideEffect()) - return true; - return false; + // TODO: Properly handle region side effects. + return !MemoryEffectOpInterface::hasNoEffect(op) || op->getNumRegions() != 0; } static void propagateLiveness(Region ®ion, LiveMap &liveMap); diff --git a/mlir/test/Transforms/canonicalize.mlir b/mlir/test/Transforms/canonicalize.mlir index 7466054..877693b 100644 --- a/mlir/test/Transforms/canonicalize.mlir +++ b/mlir/test/Transforms/canonicalize.mlir @@ -445,8 +445,6 @@ func @dim_op_fold(%arg0: index, %arg1: index, %arg2: index, %BUF: memref<?xi8>, } } } - // CHECK-NEXT: %c0 = constant 0 : index - // CHECK-NEXT: %c1 = constant 1 : index // CHECK-NEXT: affine.for %arg7 = 0 to %arg2 { // CHECK-NEXT: affine.for %arg8 = 0 to %arg0 { // CHECK-NEXT: affine.for %arg9 = %arg0 to %arg0 { @@ -468,9 +466,7 @@ func @dim_op_fold(%arg0: index, %arg1: index, %arg2: index, %BUF: memref<?xi8>, } } } - // CHECK: loop.for %{{.*}} = %c0 to %[[M]] step %c1 { - // CHECK: loop.for %arg8 = %c0 to %[[N]] step %c1 { - // CHECK: loop.for %arg9 = %c0 to %[[K]] step %c1 { + // CHECK-NEXT: return return } diff --git a/mlir/test/mlir-tblgen/op-decl.td b/mlir/test/mlir-tblgen/op-decl.td index a6719cf..7606c33 100644 --- a/mlir/test/mlir-tblgen/op-decl.td +++ b/mlir/test/mlir-tblgen/op-decl.td @@ -10,9 +10,9 @@ def Test_Dialect : Dialect { class NS_Op<string mnemonic, list<OpTrait> traits> : Op<Test_Dialect, mnemonic, traits>; -// NoSideEffect trait is included twice to ensure it gets uniqued during +// IsolatedFromAbove trait is included twice to ensure it gets uniqued during // emission. -def NS_AOp : NS_Op<"a_op", [NoSideEffect, NoSideEffect]> { +def NS_AOp : NS_Op<"a_op", [IsolatedFromAbove, IsolatedFromAbove]> { let arguments = (ins I32:$a, Variadic<F32>:$b, @@ -55,7 +55,8 @@ def NS_AOp : NS_Op<"a_op", [NoSideEffect, NoSideEffect]> { // CHECK: ArrayRef<Value> tblgen_operands; // CHECK: }; -// CHECK: class AOp : public Op<AOp, OpTrait::AtLeastNResults<1>::Impl, OpTrait::ZeroSuccessor, OpTrait::AtLeastNOperands<1>::Impl, OpTrait::HasNoSideEffect +// CHECK: class AOp : public Op<AOp, OpTrait::AtLeastNResults<1>::Impl, OpTrait::ZeroSuccessor, OpTrait::AtLeastNOperands<1>::Impl, OpTrait::IsIsolatedFromAbove +// CHECK-NOT: OpTrait::IsIsolatedFromAbove // CHECK: public: // CHECK: using Op::Op; // CHECK: using OperandAdaptor = AOpOperandAdaptor; diff --git a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp index 853f399..8953bcbc 100644 --- a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp +++ b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp @@ -1184,15 +1184,21 @@ void OpEmitter::genSideEffectInterfaceMethods() { unsigned index, unsigned kind) { for (auto decorator : decorators) if (SideEffect *effect = dyn_cast<SideEffect>(&decorator)) - interfaceEffects[effect->getInterfaceTrait()].push_back( + interfaceEffects[effect->getBaseEffectName()].push_back( EffectLocation{*effect, index, kind}); }; // Collect effects that were specified via: /// Traits. - for (const auto &trait : op.getTraits()) - if (const auto *opTrait = dyn_cast<tblgen::SideEffectTrait>(&trait)) - resolveDecorators(opTrait->getEffects(), /*index=*/0, EffectKind::Static); + for (const auto &trait : op.getTraits()) { + const auto *opTrait = dyn_cast<tblgen::SideEffectTrait>(&trait); + if (!opTrait) + continue; + auto &effects = interfaceEffects[opTrait->getBaseEffectName()]; + for (auto decorator : opTrait->getEffects()) + effects.push_back(EffectLocation{cast<SideEffect>(decorator), + /*index=*/0, EffectKind::Static}); + } /// Operands. for (unsigned i = 0, operandIt = 0, e = op.getNumArgs(); i != e; ++i) { if (op.getArg(i).is<NamedTypeConstraint *>()) { @@ -1205,11 +1211,10 @@ void OpEmitter::genSideEffectInterfaceMethods() { resolveDecorators(op.getResultDecorators(i), i, EffectKind::Result); for (auto &it : interfaceEffects) { - StringRef baseEffect = it.second.front().effect.getBaseName(); auto effectsParam = llvm::formatv( "SmallVectorImpl<SideEffects::EffectInstance<{0}>> &effects", - baseEffect) + it.first()) .str(); // Generate the 'getEffects' method. |