[Mlir-commits] [mlir] [MLIR][SideEffects][MemoryEffects] Modified LICM to be more aggressive when checking movability of ops with MemWrite effects (PR #155344)
llvmlistbot at llvm.org
llvmlistbot at llvm.org
Fri Sep 12 13:14:55 PDT 2025
github-actions[bot] wrote:
<!--LLVM CODE FORMAT COMMENT: {clang-format}-->
:warning: C/C++ code formatter, clang-format found issues in your code. :warning:
<details>
<summary>
You can test this locally with the following command:
</summary>
``````````bash
git-clang-format --diff origin/main HEAD --extensions cpp,h -- mlir/include/mlir/Interfaces/SideEffectInterfaces.h mlir/include/mlir/Transforms/LoopInvariantCodeMotionUtils.h mlir/lib/Interfaces/SideEffectInterfaces.cpp mlir/lib/Transforms/Utils/LoopInvariantCodeMotionUtils.cpp mlir/test/lib/Dialect/Test/TestOps.h
``````````
:warning:
The reproduction instructions above might return results for more than one PR
in a stack if you are using a stacked PR workflow. You can limit the results by
changing `origin/main` to the base branch/commit you want to compare against.
:warning:
</details>
<details>
<summary>
View the diff from clang-format here.
</summary>
``````````diff
diff --git a/mlir/include/mlir/Interfaces/SideEffectInterfaces.h b/mlir/include/mlir/Interfaces/SideEffectInterfaces.h
index e2b3ba10e..cb705a06e 100644
--- a/mlir/include/mlir/Interfaces/SideEffectInterfaces.h
+++ b/mlir/include/mlir/Interfaces/SideEffectInterfaces.h
@@ -383,7 +383,9 @@ getMemoryEffectsSorted(Operation *op);
/// resource. An 'allocate' effect implies only allocation of the resource, and
/// not any visible mutation or dereference.
struct Allocate : public Effect::Base<Allocate> {
- Allocate() : Effect::Base<Allocate>() { this->priority = Priority::kAllocPriority; }
+ Allocate() : Effect::Base<Allocate>() {
+ this->priority = Priority::kAllocPriority;
+ }
};
/// The following effect indicates that the operation frees some resource that
diff --git a/mlir/include/mlir/Transforms/LoopInvariantCodeMotionUtils.h b/mlir/include/mlir/Transforms/LoopInvariantCodeMotionUtils.h
index 82581efda..ef7c72e66 100644
--- a/mlir/include/mlir/Transforms/LoopInvariantCodeMotionUtils.h
+++ b/mlir/include/mlir/Transforms/LoopInvariantCodeMotionUtils.h
@@ -25,14 +25,16 @@ class Value;
/// Gathers potential conflicts on all memory resources used within loop
///
-/// Given a target loop and an op within it (or the loop op itself),
-/// gathers op's memory effects and flags potential resource conflicts
-/// in a map and then recurses into the op's regions to gather nested
-/// resource conflicts
+/// Given a target loop and an op within it (or the loop op itself),
+/// gathers op's memory effects and flags potential resource conflicts
+/// in a map and then recurses into the op's regions to gather nested
+/// resource conflicts
///
/// First call should use loop = someLoop and op = someLoop.getOperation()
-void gatherResourceConflicts(LoopLikeOpInterface loop, Operation *op,
- DenseMap<TypeID, std::pair<bool, MemoryEffects::EffectInstance>> &resourceConflicts);
+void gatherResourceConflicts(
+ LoopLikeOpInterface loop, Operation *op,
+ DenseMap<TypeID, std::pair<bool, MemoryEffects::EffectInstance>>
+ &resourceConflicts);
/// Given a list of regions, perform loop-invariant code motion. An operation is
/// loop-invariant if it depends only of values defined outside of the loop.
diff --git a/mlir/lib/Interfaces/SideEffectInterfaces.cpp b/mlir/lib/Interfaces/SideEffectInterfaces.cpp
index ec8618e24..6603f9193 100644
--- a/mlir/lib/Interfaces/SideEffectInterfaces.cpp
+++ b/mlir/lib/Interfaces/SideEffectInterfaces.cpp
@@ -330,19 +330,19 @@ mlir::MemoryEffects::getMemoryEffectsSorted(Operation *op) {
memInterface.getEffects(effectsSorted);
- auto sortEffects =
- [](llvm::SmallVectorImpl<MemoryEffects::EffectInstance> &effects) {
- llvm::stable_sort(effects, [](const MemoryEffects::EffectInstance &a,
- const MemoryEffects::EffectInstance &b) {
- if (a.getStage() < b.getStage())
- return true;
-
- if (a.getStage() == b.getStage())
- return a.getEffect()->getPriority() < b.getEffect()->getPriority();
-
- return false; // b before a
- });
- };
+ auto sortEffects =
+ [](llvm::SmallVectorImpl<MemoryEffects::EffectInstance> &effects) {
+ llvm::stable_sort(effects, [](const MemoryEffects::EffectInstance &a,
+ const MemoryEffects::EffectInstance &b) {
+ if (a.getStage() < b.getStage())
+ return true;
+
+ if (a.getStage() == b.getStage())
+ return a.getEffect()->getPriority() < b.getEffect()->getPriority();
+
+ return false; // b before a
+ });
+ };
sortEffects(effectsSorted);
return effectsSorted;
@@ -352,12 +352,12 @@ bool mlir::isMemoryEffectFree(Operation *op) {
if (auto memInterface = dyn_cast<MemoryEffectOpInterface>(op)) {
if (!memInterface.hasNoEffect())
return false;
-
+
// If the op does not have recursive side effects, then it is memory effect
// free.
if (!op->hasTrait<OpTrait::HasRecursiveMemoryEffects>())
return true;
-
+
} else if (!op->hasTrait<OpTrait::HasRecursiveMemoryEffects>()) {
// Otherwise, if the op does not implement the memory effect interface and
// it does not have recursive side effects, then it cannot be known that the
diff --git a/mlir/lib/Transforms/Utils/LoopInvariantCodeMotionUtils.cpp b/mlir/lib/Transforms/Utils/LoopInvariantCodeMotionUtils.cpp
index 7b59b4abb..a2cbcc40e 100644
--- a/mlir/lib/Transforms/Utils/LoopInvariantCodeMotionUtils.cpp
+++ b/mlir/lib/Transforms/Utils/LoopInvariantCodeMotionUtils.cpp
@@ -60,18 +60,19 @@ static bool canBeHoisted(Operation *op,
op, [&](OpOperand &operand) { return definedOutside(operand.get()); });
}
-/// Merges srcEffect's Memory Effect on its resource into the
+/// Merges srcEffect's Memory Effect on its resource into the
/// resourceConflicts map, flagging resources if the srcEffect
/// results in a conflict
-static void mergeResource(
- DenseMap<TypeID, std::pair<bool, MemoryEffects::EffectInstance>> &resourceConflicts,
- const MemoryEffects::EffectInstance &srcEffect,
- bool srcHasConflict) {
+static void
+mergeResource(DenseMap<TypeID, std::pair<bool, MemoryEffects::EffectInstance>>
+ &resourceConflicts,
+ const MemoryEffects::EffectInstance &srcEffect,
+ bool srcHasConflict) {
TypeID srcResourceID = srcEffect.getResource()->getResourceID();
- bool srcIsAllocOrFree = isa<MemoryEffects::Allocate>(srcEffect.getEffect())
- || isa<MemoryEffects::Free>(srcEffect.getEffect());
+ bool srcIsAllocOrFree = isa<MemoryEffects::Allocate>(srcEffect.getEffect()) ||
+ isa<MemoryEffects::Free>(srcEffect.getEffect());
bool conflict = srcHasConflict || srcIsAllocOrFree;
@@ -79,7 +80,8 @@ static void mergeResource(
// if it doesn't already exist, create entry for resource in map
if (dstIt == resourceConflicts.end()) {
- resourceConflicts.insert(std::make_pair(srcResourceID, std::make_pair(conflict, srcEffect)));
+ resourceConflicts.insert(
+ std::make_pair(srcResourceID, std::make_pair(conflict, srcEffect)));
return;
}
@@ -93,10 +95,10 @@ static void mergeResource(
bool srcWrite = isa<MemoryEffects::Write>(srcEffect.getEffect());
bool dstRead = isa<MemoryEffects::Read>(dstEffect.getEffect());
bool readBeforeWrite = dstRead && srcWrite;
-
+
conflict = conflict || readBeforeWrite;
- dstIt->second =std::make_pair(conflict, srcEffect);
+ dstIt->second = std::make_pair(conflict, srcEffect);
}
/// Returns true if any of op's OpOperands are defined outside of loopLike
@@ -113,14 +115,16 @@ static bool hasLoopVariantInput(LoopLikeOpInterface loopLike, Operation *op) {
/// flagged as having a conflict within the resourceConflicts map
/// (b) op doesn't have a MemoryEffectOpInterface or has one but
/// without any specific effects
-static bool mayHaveMemoryEffectConflict(Operation *op,
- DenseMap<TypeID, std::pair<bool, MemoryEffects::EffectInstance>> &resourceConflicts) {
+static bool mayHaveMemoryEffectConflict(
+ Operation *op,
+ DenseMap<TypeID, std::pair<bool, MemoryEffects::EffectInstance>>
+ &resourceConflicts) {
auto memInterface = dyn_cast<MemoryEffectOpInterface>(op);
-
+
// op does not implement the memory effect op interface
// shouldn't be flagged as movable to be conservative
- if (!memInterface)
+ if (!memInterface)
return true;
// gather all effects on op
@@ -128,7 +132,7 @@ static bool mayHaveMemoryEffectConflict(Operation *op,
memInterface.getEffects(effects);
// op has interface but no effects, be conservative
- if (effects.empty())
+ if (effects.empty())
return true;
// RFC moving ops with HasRecursiveMemoryEffects that have nested ops
@@ -138,10 +142,10 @@ static bool mayHaveMemoryEffectConflict(Operation *op,
for (const MemoryEffects::EffectInstance &effect : effects) {
auto resourceID = effect.getResource()->getResourceID();
- auto resConIt = resourceConflicts.find(resourceID);
+ auto resConIt = resourceConflicts.find(resourceID);
if (resConIt == resourceConflicts.end())
return true; // RFC realistically shouldn't reach here but just in case?
-
+
bool hasConflict = resConIt->second.first;
if (hasConflict)
return true;
@@ -150,13 +154,15 @@ static bool mayHaveMemoryEffectConflict(Operation *op,
return false;
}
-void mlir::gatherResourceConflicts(LoopLikeOpInterface loopLike, Operation *op,
- DenseMap<TypeID, std::pair<bool, MemoryEffects::EffectInstance>> &resourceConflicts) {
+void mlir::gatherResourceConflicts(
+ LoopLikeOpInterface loopLike, Operation *op,
+ DenseMap<TypeID, std::pair<bool, MemoryEffects::EffectInstance>>
+ &resourceConflicts) {
if (auto memInterface = dyn_cast<MemoryEffectOpInterface>(op)) {
// gather all effects on op
llvm::SmallVector<MemoryEffects::EffectInstance> effects =
- MemoryEffects::getMemoryEffectsSorted(op);
+ MemoryEffects::getMemoryEffectsSorted(op);
if (!effects.empty()) {
// any variant input to the op could be the data source
@@ -166,7 +172,7 @@ void mlir::gatherResourceConflicts(LoopLikeOpInterface loopLike, Operation *op,
for (const MemoryEffects::EffectInstance &effect : effects) {
bool conflict = false;
bool isWrite = isa<MemoryEffects::Write>(effect.getEffect());
-
+
// all writes to a resource that follow a read on any other resource
// have to be considered a conflict as guaranteeing that the read
// is invariant and won't affect the write requires more robust logic
@@ -183,8 +189,8 @@ void mlir::gatherResourceConflicts(LoopLikeOpInterface loopLike, Operation *op,
}
for (Region ®ion : op->getRegions())
- for (Operation &opInner : region.getOps())
- gatherResourceConflicts(loopLike, &opInner, resourceConflicts);
+ for (Operation &opInner : region.getOps())
+ gatherResourceConflicts(loopLike, &opInner, resourceConflicts);
}
size_t mlir::moveLoopInvariantCode(
@@ -205,8 +211,10 @@ size_t mlir::moveLoopInvariantCode(
// continuous region --> need to add fork checking
//
// loop "do" and "then" regions also merged
- DenseMap<TypeID, std::pair<bool, MemoryEffects::EffectInstance>> resourceConflicts;
- mlir::gatherResourceConflicts(loopLike, loopLike.getOperation(), resourceConflicts);
+ DenseMap<TypeID, std::pair<bool, MemoryEffects::EffectInstance>>
+ resourceConflicts;
+ mlir::gatherResourceConflicts(loopLike, loopLike.getOperation(),
+ resourceConflicts);
auto regions = loopLike.getLoopRegions();
for (Region *region : regions) {
@@ -231,12 +239,12 @@ size_t mlir::moveLoopInvariantCode(
LDBG() << "Checking op: "
<< OpWithFlags(op, OpPrintingFlags().skipRegions());
- bool noMemoryConflicts = isMemoryEffectFree(op)
- || !mayHaveMemoryEffectConflict(op, resourceConflicts);
+ bool noMemoryConflicts =
+ isMemoryEffectFree(op) ||
+ !mayHaveMemoryEffectConflict(op, resourceConflicts);
- if (!noMemoryConflicts
- || !shouldMoveOutOfRegion(op, region)
- || !canBeHoisted(op, definedOutside))
+ if (!noMemoryConflicts || !shouldMoveOutOfRegion(op, region) ||
+ !canBeHoisted(op, definedOutside))
continue;
LDBG() << "Moving loop-invariant op: " << *op;
@@ -260,9 +268,7 @@ size_t mlir::moveLoopInvariantCode(LoopLikeOpInterface loopLike) {
[&](Value value, Region *) {
return loopLike.isDefinedOutsideOfLoop(value);
},
- [&](Operation *op, Region *) {
- return isSpeculatable(op);
- },
+ [&](Operation *op, Region *) { return isSpeculatable(op); },
[&](Operation *op, Region *) { loopLike.moveOutOfLoop(op); });
}
``````````
</details>
https://github.com/llvm/llvm-project/pull/155344
More information about the Mlir-commits
mailing list