[Mlir-commits] [mlir] 0ddba0b - [mlir][SideEffects] Replace HasNoSideEffect with the memory effect interfaces.
River Riddle
llvmlistbot at llvm.org
Thu Mar 12 14:28:01 PDT 2020
Author: River Riddle
Date: 2020-03-12T14:26:15-07:00
New Revision: 0ddba0bd59c337f16b51a00cb205ecfda46f97fa
URL: https://github.com/llvm/llvm-project/commit/0ddba0bd59c337f16b51a00cb205ecfda46f97fa
DIFF: https://github.com/llvm/llvm-project/commit/0ddba0bd59c337f16b51a00cb205ecfda46f97fa.diff
LOG: [mlir][SideEffects] Replace HasNoSideEffect with the memory effect interfaces.
HasNoSideEffect can now be implemented using the MemoryEffectInterface, removing the need to check multiple things for the same information. This also removes an easy foot-gun for users as 'Operation::hasNoSideEffect' would ignore operations that dynamically, or recursively, have no side effects. This also leads to an immediate improvement in some of the existing users, such as DCE, now that they have access to more information.
Differential Revision: https://reviews.llvm.org/D76036
Added:
Modified:
mlir/docs/Traits.md
mlir/docs/Tutorials/Toy/Ch-2.md
mlir/include/mlir/IR/Operation.h
mlir/include/mlir/IR/OperationSupport.h
mlir/include/mlir/Interfaces/SideEffects.h
mlir/include/mlir/Interfaces/SideEffects.td
mlir/include/mlir/TableGen/SideEffects.h
mlir/include/mlir/Transforms/FoldUtils.h
mlir/lib/Analysis/Utils.cpp
mlir/lib/Conversion/LoopsToGPU/LoopsToGPU.cpp
mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp
mlir/lib/Dialect/LoopOps/Transforms/ParallelLoopFusion.cpp
mlir/lib/Interfaces/SideEffects.cpp
mlir/lib/TableGen/SideEffects.cpp
mlir/lib/Transforms/CSE.cpp
mlir/lib/Transforms/LoopInvariantCodeMotion.cpp
mlir/lib/Transforms/Utils/FoldUtils.cpp
mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
mlir/lib/Transforms/Utils/LoopUtils.cpp
mlir/lib/Transforms/Utils/RegionUtils.cpp
mlir/test/Transforms/canonicalize.mlir
mlir/test/mlir-tblgen/op-decl.td
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
Removed:
################################################################################
diff --git a/mlir/docs/Traits.md b/mlir/docs/Traits.md
index e825eb6ec6db..9877d902982d 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 881e077b8395..c8be4819a277 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 781dbcd3eb0d..41838451329b 100644
--- a/mlir/include/mlir/IR/Operation.h
+++ b/mlir/include/mlir/IR/Operation.h
@@ -407,13 +407,6 @@ class Operation final
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 2d4a96823d48..892888622cd6 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 96714da08560..e0fd17590fdb 100644
--- a/mlir/include/mlir/Interfaces/SideEffects.h
+++ b/mlir/include/mlir/Interfaces/SideEffects.h
@@ -163,15 +163,6 @@ template <typename EffectT> class EffectInstance {
//===----------------------------------------------------------------------===//
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 ebf3da628ebf..8bb40728d7d1 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 c93502cc7a7a..eb2a06803d0f 100644
--- a/mlir/include/mlir/TableGen/SideEffects.h
+++ b/mlir/include/mlir/TableGen/SideEffects.h
@@ -27,10 +27,7 @@ class SideEffect : public Operator::VariableDecorator {
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 @@ class SideEffectTrait : public InterfaceOpTrait {
// 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 56a78b1bf805..83ce3bf0d072 100644
--- a/mlir/include/mlir/Transforms/FoldUtils.h
+++ b/mlir/include/mlir/Transforms/FoldUtils.h
@@ -106,6 +106,9 @@ class OperationFolder {
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 14635a144735..7b3cd58aa980 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 293d93512147..9ba3f40d24e9 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 de298487bea7..3274abd81111 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 98f8313d9ab7..b84cfa5aa78c 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 da43239a2752..53406c6e8b32 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 0b334b8297a0..7fbeffaafbdb 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 3a7659487a47..42ba7153f5cd 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 a452e33e8f9e..73009484e7d9 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 f374d3803baa..66535ec67897 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 f9a9be533931..e40a4d998221 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 a02c71af3c00..7095e55eca6a 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 78bb609b5c03..162091cd53de 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 746605452baa..877693b78c87 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 a6719cfa5117..7606c3356140 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 853f399af5e2..8953bcbc54b9 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.
More information about the Mlir-commits
mailing list