-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[mlir] Add requiresReplacedValues
and visitReplacedValues
to PromotableOpInterface
#86792
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
e3b7f23
9e85edd
327ee45
a03e75d
975e1ca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -229,6 +229,27 @@ def PromotableOpInterface : OpInterface<"PromotableOpInterface"> { | |
(ins "const ::llvm::SmallPtrSetImpl<mlir::OpOperand *> &":$blockingUses, | ||
"::mlir::RewriterBase &":$rewriter) | ||
>, | ||
InterfaceMethod<[{ | ||
Checks whether the operation requires visiting the mutated | ||
definitions by a store operation. | ||
}], "bool", "requiresVisitingMutatedDefs", (ins), [{}], | ||
[{ return false; }] | ||
>, | ||
InterfaceMethod<[{ | ||
Visits all the mutated definitions by a store operation. | ||
|
||
This method will only be called after all blocking uses have been | ||
scheduled for removal and if `requiresVisitingMutatedDefs` returned | ||
true. | ||
|
||
The rewriter is located after the promotable operation on call. All IR | ||
mutations must happen through the rewriter. During the transformation, | ||
*no operation should be deleted*. | ||
}], | ||
"void", "visitMutatedDefs", | ||
(ins "::llvm::ArrayRef<std::pair<::mlir::Operation*, ::mlir::Value>>":$mutatedDefs, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Name suggestion: |
||
"::mlir::RewriterBase &":$rewriter), [{}], [{ return; }] | ||
Comment on lines
+259
to
+260
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like to be absolutely complete this method should also provide the memory slot, as one may want to know which memory slot specifically those definitions correspond to. But at the same time I am not really sure that this would be useful in practice, so change it only if you feel like it. |
||
>, | ||
]; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -202,6 +202,7 @@ class MemorySlotPromoter { | |
/// Contains the reaching definition at this operation. Reaching definitions | ||
/// are only computed for promotable memory operations with blocking uses. | ||
DenseMap<PromotableMemOpInterface, Value> reachingDefs; | ||
DenseMap<PromotableMemOpInterface, Value> mutatedValues; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really dislike the "mutatedValues" name because it is unclear to me in what way those values are mutated. This name suggests you put something in those values when what happens conceptually is the opposite. |
||
DominanceInfo &dominance; | ||
MemorySlotPromotionInfo info; | ||
const Mem2RegStatistics &statistics; | ||
|
@@ -438,6 +439,7 @@ Value MemorySlotPromoter::computeReachingDefInBlock(Block *block, | |
assert(stored && "a memory operation storing to a slot must provide a " | ||
"new definition of the slot"); | ||
reachingDef = stored; | ||
mutatedValues[memOp] = stored; | ||
} | ||
} | ||
} | ||
|
@@ -552,6 +554,8 @@ void MemorySlotPromoter::removeBlockingUses() { | |
dominanceSort(usersToRemoveUses, *slot.ptr.getParentBlock()->getParent()); | ||
|
||
llvm::SmallVector<Operation *> toErase; | ||
llvm::SmallVector<std::pair<Operation *, Value>> mutatedDefinitions; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above for this name. |
||
llvm::SmallVector<PromotableOpInterface> visitMutatedDefinitions; | ||
for (Operation *toPromote : llvm::reverse(usersToRemoveUses)) { | ||
if (auto toPromoteMemOp = dyn_cast<PromotableMemOpInterface>(toPromote)) { | ||
Value reachingDef = reachingDefs.lookup(toPromoteMemOp); | ||
|
@@ -565,7 +569,9 @@ void MemorySlotPromoter::removeBlockingUses() { | |
slot, info.userToBlockingUses[toPromote], rewriter, | ||
reachingDef) == DeletionKind::Delete) | ||
toErase.push_back(toPromote); | ||
|
||
if (toPromoteMemOp.storesTo(slot)) | ||
if (Value mutatedValue = mutatedValues[toPromoteMemOp]) | ||
mutatedDefinitions.push_back({toPromoteMemOp, mutatedValue}); | ||
continue; | ||
} | ||
|
||
|
@@ -574,6 +580,12 @@ void MemorySlotPromoter::removeBlockingUses() { | |
if (toPromoteBasic.removeBlockingUses(info.userToBlockingUses[toPromote], | ||
rewriter) == DeletionKind::Delete) | ||
toErase.push_back(toPromote); | ||
if (toPromoteBasic.requiresVisitingMutatedDefs()) | ||
visitMutatedDefinitions.push_back(toPromoteBasic); | ||
} | ||
for (PromotableOpInterface op : visitMutatedDefinitions) { | ||
rewriter.setInsertionPointAfter(op); | ||
op.visitMutatedDefs(mutatedDefinitions, rewriter); | ||
} | ||
|
||
for (Operation *toEraseOp : toErase) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain what this means more in the documentation?