-
Notifications
You must be signed in to change notification settings - Fork 802
[SYCL] Add queue properties to select submission mode #9554
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
Conversation
sycl/include/sycl/detail/pi.h
Outdated
| constexpr pi_queue_properties PI_EXT_ONEAPI_QUEUE_FLAG_PRIORITY_LOW = (1 << 5); | ||
| constexpr pi_queue_properties PI_EXT_ONEAPI_QUEUE_FLAG_PRIORITY_HIGH = (1 << 6); | ||
| constexpr pi_queue_properties PI_EXT_ONEAPI_QUEUE_FLAG_BATCHED_SUBMISSION = (1 << 7); | ||
| constexpr pi_queue_properties PI_EXT_ONEAPI_QUEUE_FLAG_IMMEDIATE_SUBMISSION = (1 << 8); |
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.
As this PR changes pi.h then it will require UR changes to match this.
As stated in a comment on another PR, documentation on how to add experimental features is being written here oneapi-src/unified-runtime#546
But this change may be small enough and concrete enough to skip experimental and we just add it to UR directly?
You can ask us (@alycm, @jandres742, @kbenzie) any questions and we'll help you out.
|
How is this supposed to work on non-Intel GPUs? |
|
The queue properties are intended to be a hint to the implementation. On L0, where we have different submission modes, it is clear what to do. On other platforms the implementation may ignore these hints if they cannot be mapped to something useful. |
| else if (isImmediateSubmission()) | ||
| UsingImmCmdLists = true; | ||
| else | ||
| UsingImmCmdLists = Device->useImmediateCommandLists(); |
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.
the environment variable should have priority over the flags, because that way, the user can force one or the other behavior w/o having to change the code.
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.
In this case, the code, if it uses an explicit queue property, should win. Only if the queue property is not used should we rely on env vars.
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.
@rdeodhar : but let's say you have the binary for a customer application taht uses batched submission, and you want to see if a functional or performance bug is gone if you used immediate submission? by having the environment variable always taking priority, you can change the behavior and debug the issue w/o needing a new binary.
same from point of view of user: Imagine the user has a very large codebase, where immediate submission is used, and they want just to see what would happen if batched submission were used. They could go and change all the codebase, but that would mean time refactoring, compiling and executing, while with the environment variable, they can test and see any benefits or problems before investing time changing the code.
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 appreciate your reasoning but when something is explicitly (hard) coded in the program it should not be subject to over-ride through env vars. Do you know of any other SYCL properties that are subject to over-ride through env vars?
A secondary question is whether the env var has been explicitly set or default. Should even a default setting of the env var over-ride the code? What about explicit setting to what was already the default setting? Should that be considered an explicit setting and over-ride what is in the code?
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.
@rdeodhar for the second question, we would need to have a default value of -1 for the environment variable, so something like this:
if (environment variable != -1) {
set queue mode to value set by environment variable;
} else if (batched mode selected by application) {
set batched mode
} else if (immediate mode selected by application) {
set immediate mode
} else {
set defaults // which currently is for instance immediate mode for PVC and batched for others
}so again, this is mostly about helping with the debug and programmability. but it is not a hard blocker. If you think it is better to have priority in the setting from application, it is good too.
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.
The compiler convention is to honor explicit coding and allow env var control over other behavior. So we'll stick with the way its done now.
KseniyaTikhomirova
left a comment
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.
LGTM
sycl/source/detail/queue_impl.hpp
Outdated
| bool SubmissionSeen = false; | ||
| if (PropList | ||
| .has_property<ext::oneapi::property::queue::batched_submission>()) { | ||
| if (SubmissionSeen) { |
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.
seems to be redundant here. it is the first place Submission seen could be set to true
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.
Yes, you are right. Removed unneeded code.
sycl/doc/extensions/supported/sycl_ext_intel_queue_submission_mode.asciidoc
Outdated
Show resolved
Hide resolved
sycl/doc/extensions/supported/sycl_ext_intel_queue_submission_mode.asciidoc
Outdated
Show resolved
Hide resolved
sycl/doc/extensions/supported/sycl_ext_intel_queue_submission_mode.asciidoc
Outdated
Show resolved
Hide resolved
sycl/doc/extensions/supported/sycl_ext_intel_queue_submission_mode.asciidoc
Outdated
Show resolved
Hide resolved
sycl/doc/extensions/supported/sycl_ext_intel_queue_submission_mode.asciidoc
Outdated
Show resolved
Hide resolved
sycl/doc/extensions/supported/sycl_ext_intel_queue_submission_mode.asciidoc
Outdated
Show resolved
Hide resolved
sycl/doc/extensions/supported/sycl_ext_intel_queue_immediate_command_list.asciidoc
Outdated
Show resolved
Hide resolved
sycl/doc/extensions/supported/sycl_ext_intel_queue_immediate_command_list.asciidoc
Show resolved
Hide resolved
sycl/doc/extensions/supported/sycl_ext_intel_queue_immediate_command_list.asciidoc
Show resolved
Hide resolved
gmlueck
left a comment
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.
extension spec LGTM.
This change adds the queue properties immediate_command_list and no_immediate_command_list to enable overriding the queue submission defaults.
This change adds the queue properties immediate_command_list and no_immediate_command_list to enable overriding the queue submission defaults.