-
Couldn't load subscription status.
- Fork 15k
[mlir][memref] Introduce memref.distinct_objects op
#156913
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 3 commits
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 |
|---|---|---|
|
|
@@ -542,6 +542,29 @@ OpFoldResult AssumeAlignmentOp::fold(FoldAdaptor adaptor) { | |
| return getMemref(); | ||
| } | ||
|
|
||
| //===----------------------------------------------------------------------===// | ||
| // DistinctObjectsOp | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| LogicalResult DistinctObjectsOp::verify() { | ||
| if (getOperandTypes() != getResultTypes()) | ||
| return emitOpError("operand types and result types must match"); | ||
|
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. Nit: Should be implementable with the 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. We have a variadic number of inputs/results and we need their types to match pairwise. I didn't quite figured how to express it using existing constraints. 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. Oh I missed that they are variadics! Makes sense. 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. Can you add a test though? It a custom verifier, so we should test it. 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. added a test |
||
|
|
||
| if (getOperandTypes().empty()) | ||
| return emitOpError("expected at least one operand"); | ||
|
|
||
| return success(); | ||
| } | ||
|
|
||
| LogicalResult DistinctObjectsOp::inferReturnTypes( | ||
|
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. Can this be done with 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 don't know how to express it using existing constraints, see my prev comment on 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. We could probably add a constraint - something like EachTypeMatches<"src", "res"> - and possibly use it to do return type inference But that's a separate PR |
||
| MLIRContext * /*context*/, std::optional<Location> /*location*/, | ||
| ValueRange operands, DictionaryAttr /*attributes*/, | ||
| OpaqueProperties /*properties*/, RegionRange /*regions*/, | ||
| SmallVectorImpl<Type> &inferredReturnTypes) { | ||
| llvm::copy(operands.getTypes(), std::back_inserter(inferredReturnTypes)); | ||
| return success(); | ||
| } | ||
|
|
||
| //===----------------------------------------------------------------------===// | ||
| // CastOp | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
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.
Copying comment from the other discussion
@krzysz00
That's a good question, imagine we have code like
If we just use
bufferPtrwithout the offset we will en up withassume separate_storage(%ptr, %ptr), i.e. same pointer, which makes no sense.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 have another PR in works #160512, which translates
memref.distinct_objectsinto alias scope attributes on loads/stores which shouldn't have this problem.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.
bufferPtris the version that adds in the offsetThere 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, srory, I misunderstood the issue then, I took this code from
AssumeAlignmentOpLoweringlowering, so I suppose it has the same issue?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.
Yeah, probably
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.
fixed, but I believe resulting LLVM IR will be exactly the same.