-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[mlir][memref][llvm] Infer llvm alias scopes attrs from memref.distinct_objects
#160512
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
base: main
Are you sure you want to change the base?
Conversation
}]; | ||
} | ||
|
||
def DistinctObjectsInterface : OpInterface<"DistinctObjectsInterface"> { |
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.
Do we need to preemptively define an interface here?
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.
I added an interface so MLIR LocalAliasAnalysis
won't depend directly on memref dialect.
for (auto [origOperand, newOperand] : | ||
llvm::zip_equal(op.getOperands(), operands)) { | ||
auto memrefType = cast<MemRefType>(origOperand.getType()); | ||
Value ptr = getStridedElementPtr(rewriter, loc, memrefType, newOperand, |
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.
I'm not sure this is the right thing? I think we want bufferPtr
on MemRefDescriptor
.
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.
(this code is taken from original distinct_objects
PR, please comment there)
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.
Ah - you haven't pushed to a users/ to set up a stacked branch
Can you pull my comment over?
static SmallVector<std::pair<StringAttr, Attribute>> | ||
copyImportantAttrs(Operation *op) { | ||
SmallVector<std::pair<StringAttr, Attribute>> attrs; | ||
for (StringRef attrName : {LLVM::LLVMDialect::getAliasScopesAttrName(), |
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.
Can we just unconditionally pass through LLVM dialect attributes here?
op, adaptor.getValue(), dataPtr, op.getAlignment().value_or(0), false, | ||
op.getNontemporal()); | ||
|
||
for (auto [nameAttr, attr] : importantAttrs) |
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.
You don't need the for loop, I think, or putting the extraction beforehand. You could just setDiscardableAttrs()
from a filtered list or something to that effect.
struct LLVMInferAliasScopeAttrs | ||
: public LLVM::impl::LLVMInferAliasScopeAttributesBase< | ||
LLVMInferAliasScopeAttrs> { | ||
void runOnOperation() override { |
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.
Does this end up working because alias analysis knows about separate_storage now?
And re quadratic ... union-find, maybe? (LLVM calls it EquivalenceClasses - you could sort the pointers by whether they alias existing representatives?)
Also, why's this in the LLVM directory when it's doing a lot of work with memrefs?
No description provided.