-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[mlir][memref] Allow out-of-bounds semantics for memref.subview #152164
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?
Changes from all 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 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -2047,11 +2047,9 @@ def SubViewOp : MemRef_OpWithOffsetSizesAndStrides<"subview", [ | |||||||||||
result_offset = src_offset + dot_product(offset_operands, src_strides) | ||||||||||||
``` | ||||||||||||
|
||||||||||||
The offset, size and stride operands must be in-bounds with respect to the | ||||||||||||
source memref. When possible, the static operation verifier will detect | ||||||||||||
out-of-bounds subviews. Subviews that cannot be confirmed to be in-bounds | ||||||||||||
or out-of-bounds based on compile-time information are valid. However, | ||||||||||||
performing an out-of-bounds subview at runtime is undefined behavior. | ||||||||||||
The operation does not guarantee if the created subview is in-bounds. It is | ||||||||||||
the responsibility of the user to guarantee there are no out-of-bounds | ||||||||||||
accesses into the subview. | ||||||||||||
Comment on lines
+2050
to
+2052
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. Shall we use poison?
Suggested change
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. @Groverkss Can you describe the use case that you have in mind? If the result is poisoned (which would make sense to me), loading from it will 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. The returned value is partially poisoned. The entire memref is not poisoned. It is not undefined behavior to access part of the subview that is in bounds. |
||||||||||||
|
||||||||||||
Example 1: | ||||||||||||
|
||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,7 +76,8 @@ SliceBoundsVerificationResult verifyInBoundsSlice( | |
/// returns the new result type of the op, based on the new offsets, sizes and | ||
/// strides. `CastOpFunc` is used to generate a cast op if the result type of | ||
/// the op has changed. | ||
template <typename OpType, typename ResultTypeFn, typename CastOpFunc> | ||
template <typename OpType, typename ResultTypeFn, typename CastOpFunc, | ||
bool CheckInBounds = true> | ||
class OpWithOffsetSizesAndStridesConstantArgumentFolder final | ||
: public OpRewritePattern<OpType> { | ||
public: | ||
|
@@ -94,12 +95,14 @@ class OpWithOffsetSizesAndStridesConstantArgumentFolder final | |
failed(foldDynamicIndexList(mixedStrides))) | ||
return failure(); | ||
|
||
// Pattern does not apply if the produced op would not verify. | ||
SliceBoundsVerificationResult sliceResult = verifyInBoundsSlice( | ||
cast<ShapedType>(op.getSource().getType()).getShape(), mixedOffsets, | ||
mixedSizes, mixedStrides); | ||
if (!sliceResult.isValid) | ||
return failure(); | ||
if (CheckInBounds) { | ||
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. The conditional bounds checking introduces complexity where the template parameter controls verification behavior. Consider documenting why bounds checking is optional and when each mode should be used to improve code maintainability. Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||
// Pattern does not apply if the produced op would not verify. | ||
SliceBoundsVerificationResult sliceResult = verifyInBoundsSlice( | ||
cast<ShapedType>(op.getSource().getType()).getShape(), mixedOffsets, | ||
mixedSizes, mixedStrides); | ||
if (!sliceResult.isValid) | ||
return failure(); | ||
} | ||
|
||
// Compute the new result type. | ||
auto resultType = | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -745,22 +745,6 @@ func.func @invalid_subview(%arg0 : index, %arg1 : index, %arg2 : index) { | |
|
||
// ----- | ||
|
||
func.func @invalid_subview(%arg0: memref<10xf32>) { | ||
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 move these to |
||
// expected-error@+1 {{offset 0 is out-of-bounds: 10 >= 10}} | ||
%0 = memref.subview %arg0 [10][1][1] : memref<10xf32> to memref<1xf32, strided<[1], offset: 10>> | ||
return | ||
} | ||
|
||
// ----- | ||
|
||
func.func @invalid_subview(%arg0: memref<9xf32>) { | ||
// expected-error@+1 {{slice along dimension 0 runs out-of-bounds: 9 >= 9}} | ||
%0 = memref.subview %arg0 [3][4][2] : memref<9xf32> to memref<4xf32, strided<[2], offset: 3>> | ||
return | ||
} | ||
|
||
// ----- | ||
|
||
func.func @invalid_rank_reducing_subview(%arg0 : index, %arg1 : index, %arg2 : index) { | ||
%0 = memref.alloc() : memref<8x16x4xf32> | ||
// expected-error@+1 {{expected result type to be 'memref<8x16x4xf32, strided<[64, 4, 1]>>' or a rank-reduced version. (mismatch of result sizes)}} | ||
|
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 clarify what exactly out-of-bounds means? Something along the lines of: