-
Notifications
You must be signed in to change notification settings - Fork 754
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
[SYCL][Graph] Promote graph spec status to experimental #10473
Conversation
* [SYCL][Graph] Fix broken link * [SYCL][Graph] Addressing feedback from review * [SYCL][Graph] Bump version and add missing known issue * [SYCL][Graph] Add new exception throwing for unsupported backend to the Spec (#281) --------- Co-authored-by: Maxime France-Pillois <[email protected]> Co-authored-by: Ben Tracy <[email protected]>
* Add `sycl_ext_oneapi_bindless_images` as an unsupported extension. See intel#10609 * Fix host-tasks link, as we now have two sections with this heading. * Add Maxime as contributor to revision 2. * Minor formatting.
sycl/doc/extensions/experimental/sycl_ext_oneapi_graph.asciidoc
Outdated
Show resolved
Hide resolved
sycl/doc/extensions/experimental/sycl_ext_oneapi_graph.asciidoc
Outdated
Show resolved
Hide resolved
- Move unimplemented features to top of section - Add some text explaining the contents of the section - Separate fully and partially developed features into subsections for readability
* [SYCL][Graph] Add property for allowing host data created buffers - Add wording which restricts buffers created with host data in graphs - Add a property which disables those checks * [SYCL][Graph] Remove no_host_copy property * [SYCL][Graph] Add property for assuming buffer lfietimes outlive graph - Add new property for allowing buffers with a lifetime promise - Rework buffer limitations wording for this and general improvement * [SYCL][Graph] Addressing PR feedback - Add missing API modifications for properties - Add punctuation - Fix new property links * Apply suggestions from code review Co-authored-by: Pablo Reble <[email protected]> * Link to "Buffer Limitations" section --------- Co-authored-by: Ewan Crawford <[email protected]> Co-authored-by: Pablo Reble <[email protected]>
We removed the `device` namespace from `graph_support_level` in #255 However, I forgot to update this table entry.
Update the list of unimplemented features in the spec to reflect what we have fixes for (or in-progress) in the bugfix window. Also make it explicit that for unimplemented features we now throw an exception, rather than it being undefined behaviour.
While revisiting the graph fusion proposal in order to update it to match the merged version of graphs, I noticed a few small issues:
namespace sycl::ext::oneapi::experimental::property::node
class depends_on {
public:
template<typename... NodeTN>
depends_on(NodeTN... nodes);
};
} |
Update the buffer example in the spec to respect the buffer restrictions we currently have. * Use lifetime properties in constructor * Use `set_write_back(false)` on buffers used, and use host accessors to check the result. * Make buffer lifetimes exceed that of the graph object. Addresses Lukas feedback on upstream PR intel#10473 (comment)
Update the buffer example in the spec to respect the buffer restrictions we currently have. * Use lifetime properties in constructor * Use `set_write_back(false)` on buffers used, and use host accessors to check the result. * Make buffer lifetimes exceed that of the graph object. Addresses Lukas feedback on upstream PR intel#10473 (comment)
@@ -429,7 +436,7 @@ Table {counter: tableNumber}. Device Info Queries. | |||
| Device Descriptors | Return Type | Description | |||
|
|||
|`info::device::graph_support` | |||
|`info::device::graph_support_level` | |||
|`info::graph_support_level` |
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.
Sorry, I just noticed that graph_support_level
is in the info
namespace. That doesn't make sense. The info
namespace is reserved for information descriptors, which this is not. I think graph_suppport_level
should instead be in sycl::ext::oneapi::experimental
.
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 precedent we were basing a info::device::graph_support_level
query returning info::graph_support_level
on in the Core SYCL spec was info::device::device_type
descriptor returning a info::device_type
enum class, or info::device::global_mem_cache_type
returning a info::global_mem_cache_type
enum class.
Made this suggested change to remove the info
namespace from sycl::ext::oneapi::experimental::info::graph_support_level
, but curious what's different about graph_support_level
from info::device_type
and info::global_mem_cache_type
that means it's not classed as an information descriptor?
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.
Discovered that changing experimental::info::graph_support_level
to experimental::graph_support_level
in our implementation is an ABI breaking change, as a symbol with the old namespace was exported from DPC++ at the end of the last window. The next window for ABI breaking changes is currently April, but hopefully we can get an exception based on the wording
Note: Features clearly marked as experimental are considered as an exception to this guideline.
If we can't get an exception, then depending on the timelines we want this extension to "experimental" by, and how big an issue this namespacing is, we could potentially keep it as info::graph_support_level
with a spec issue to update it.
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 precedent we were basing a
info::device::graph_support_level
query returninginfo::graph_support_level
on in the Core SYCL spec wasinfo::device::device_type
descriptor returning ainfo::device_type
enum class
This is a good observation. You convinced me that it is OK to keep info::graph_support_level
. So, either way is OK with me. We seem to have precedents for both styles.
sycl/doc/extensions/experimental/sycl_ext_oneapi_graph.asciidoc
Outdated
Show resolved
Hide resolved
Addressed Greg's most recent feedback points: * [Don't need the assume_data_outlives_buffer](intel#10473 (comment)) * [Move graph_support_level namespace](intel#10473 (comment))
Addressed Greg's most recent feedback points: * [Don't need the assume_data_outlives_buffer property](intel#10473 (comment)) * [Move graph_support_level namespace](intel#10473 (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.
This looks good. I leave it up to you to decide if you want to change back to info::graph_support_level
.
…cification (#10775) This PR contains a set of changes that implement throwing an exception on invalid usage, as defined by [sycl_ext_oneapi_graph](https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/proposed/sycl_ext_oneapi_graph.asciidoc). Covering the cases missing from the current implementation. * Error checking for `make_edge` API. * Throw exception when explicit add called on a graph recording a queue. * Throw exception when creating a graph for an unsupported backend. * Error on invalid buffer behaviour when used with graphs to reflect #10473 ## Authors Co-authored-by: Pablo Reble <[email protected]> Co-authored-by: Julian Miller <[email protected]> Co-authored-by: Ben Tracy <[email protected]> Co-authored-by: Ewan Crawford <[email protected]> Co-authored-by: Maxime France-Pillois <[email protected]>
This patch updates the status of the SYCL Graph extension from proposed to experimental (after 72341ee).
Including an additional section on unimplemented features.
See https://github.com/reble/llvm#implementation-status for the status
of our total work.