-
Notifications
You must be signed in to change notification settings - Fork 15k
[Matrix][IR] Cap stride bitwidth at 64 #163729
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 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 |
|---|---|---|
|
|
@@ -6479,9 +6479,12 @@ void Verifier::visitIntrinsicCall(Intrinsic::ID ID, CallBase &Call) { | |
| NumRows->getZExtValue() * NumColumns->getZExtValue(), | ||
| "Result of a matrix operation does not fit in the returned vector!"); | ||
|
|
||
| if (Stride) | ||
| if (Stride) { | ||
| Check(Stride->getBitWidth() <= 64, "Stride bitwidth cannot exceed 64!", | ||
|
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. this looks fine to me. Are there any cases where it could be less than 64 where we might want this to be configurable? 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. Yes, in practice on targets with 32 bit index width (or lower), it may be smaller, matching the index width 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 understood Farzon's question as asking whether we might want the cap to be configurable so that, e.g., we can reject 64-bit strides when verifying IR targeting 32-bit platforms 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 restriction in the verifier is mostly just so we can use getZExtValue() on it. Regardless of the type width, stride, stride, columns, rows must be such that we can access all data without wrapping the address space. Restricting to i32 on 32 bit targets wouldn't really help to enforce that, as it would still be possible to provide arguments that would cause the accesses to wrap. 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. Makes sense, thanks! |
||
| IF); | ||
| Check(Stride->getZExtValue() >= NumRows->getZExtValue(), | ||
| "Stride must be greater or equal than the number of rows!", IF); | ||
| } | ||
|
|
||
| break; | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.