-
Notifications
You must be signed in to change notification settings - Fork 631
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
[codegen] Add max_workgroup_counts to TargetWgpAttr #17771
Conversation
ebb8cf0
to
329c1f0
Compare
329c1f0
to
43e0390
Compare
7e5cc1f
to
3caef32
Compare
@@ -118,6 +121,10 @@ TargetAttr createTargetAttr(const TargetDetails &details, StringRef arch, | |||
|
|||
//===----------------------------------------------------------------------===// | |||
// Known AMD target details | |||
// | |||
// Note: the max workgroup size is given as signed int32 max because MLIR's |
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.
What's the limit reported by rocm-smi
? (Although I think rocm-smi
reported number doesn't reflect the reality iirc?) We can make the field as int64 or optional if necessary.
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.
While rocm-smi
does report 0xff_ff_ff_ff
, that's a problem in MLIR, because we use index
for the workgroup ID, which is signed.
(I'm also not sure if that number is in units of workgroups or workitems, so it might be 0xff_ff_ff_ff / work_group_size
)
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 thinking we may want to make the field optional and interpret missing as int32 max then.. doesn't help much to explicitly print out int32 max.
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.
Hm, yeah, I'd be open to doing that
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.
let's do it as a follow up later. not blocking.
877a542
to
b92a6ce
Compare
This commit adds a max_workgroup_counts to the workgroup processor information attribute and sets values for the known targets. Some of these values may be underestimates as I was not able to locate information on their values. This field is added so that we can annotate calls to workgroup.id and workgroup.count with upper bounds, neabling range inference and strength reduction. Note that in some cases (for instance, AMD) we give a max_workgroup_counts value lower than what is actually supported because a grid dimension greater than int32_max would be sign-extended to a negative number to meet the 64-bit nature of `index`. (This PR is split out of iree-org#17707) Signed-off-by: Krzysztof Drewniak <[email protected]>
b92a6ce
to
3ce57c0
Compare
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.
LG; sorry forgot to unblock before..
BTW please feel free to directly ping me via chats for re-reviews. I'm a bit bad at tracking GitHub notifications.. |
This commit adds a max_workgroup_counts to the workgroup processor information attribute and sets values for the known targets. Some of these values may be underestimates as I was not able to locate information on their values.
This field is added so that we can annotate calls to workgroup.id and workgroup.count with upper bounds, neabling range inference and strength reduction.
Note that in some cases (for instance, AMD) we give a max_workgroup_counts value lower than what is actually supported because a grid dimension greater than int32_max would be sign-extended to a negative number to meet the 64-bit nature of
index
.(This PR is split out of #17707)