-
Notifications
You must be signed in to change notification settings - Fork 25
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
[TKW] Move default scheduling params to common place #298
base: main
Are you sure you want to change the base?
Conversation
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.
thanks for the cleanup! Was meaning to do this, but never got to it.
ELEMS_PER_THREAD: 4, | ||
ADDRESS_SPACE: mem_space, | ||
} | ||
hyperparams.update(get_default_scheduling_params()) |
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't we just integrate into the backend directly? since we may use this area to override default variables
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.
Would be good to add it here
iree-turbine/iree/turbine/kernel/wave/scheduling/resources.py
Lines 32 to 40 in e3b6c87
def get_available_resources() -> list[int]: | |
resources = [ | |
GLOBAL_MEMORY_UNITS, | |
SHARED_MEMORY_UNITS, | |
MMA_UNITS, | |
VALU_UNITS, | |
SHUFFLE_UNITS, | |
] | |
return np.array([int(subs_idxc(x)) for x in resources]) |
So instead of return np.array([int(subs_idxc(x)) for x in resources])
we can do something like:
default_resources = get_default_scheduling_params()
return np.array([int(subs_idxc(x)) if x in subs_map else default_resources[x] for x in resources])
and similar method for the delays as well. I think if we can do this way we would not need to update hyperparameter for every kernel and it makes overwriting default values cleaner. :)
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.
Those values are not really 'default' as they can be different for different GPUs, it's more like get_default_device_scheduling_params()
so I think it's better to set them explicitly.
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.
'default' as they can be different for different GPUs
That is true, but this problem also exist in the current method introduced in this PR. Hence, we'd most likely need to add query for the gfx architecture in the get_default_device_scheduling_params()
function.
so I think it's better to set them explicitly.
I think once we have querying gfx arch in get_default_device_scheduling_params
then we can do this in resources.py as proposed in the root comment. We'd just have to check that the symbols for the resources or delays either exist in user specified context or get_default_device_scheduling_params()
, if not, we will throw off some error saying that scheduling is not supported on the architecture by default and request user to specify values into context.
I think this is quite an important discussion, looking at it from kernel author's perspective, Wave programming would look quite a bit neater if we don't have to do get_default_device_scheduling_params()
and manually set them in every kernel, it'd be just a matter of boolean turning scheduling on or off. :)
Signed-off-by: Ivan Butygin <[email protected]>
Signed-off-by: Ivan Butygin <[email protected]>
269975f
to
61612ea
Compare
Signed-off-by: Ivan Butygin <[email protected]>
Moved default scheduling params to
get_default_scheduling_params()
.Didn't updated gemm tests, as it uses
VALU_UNITS: 8, SHUFFLE_UNITS: 8,
unlike attentionVALU_UNITS: 2, SHUFFLE_UNITS: 2
.