Skip to content
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

Merged
merged 13 commits into from
Aug 30, 2023

Conversation

reble
Copy link
Contributor

@reble reble commented Jul 19, 2023

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.

@reble reble marked this pull request as ready for review July 19, 2023 21:35
@reble reble requested a review from a team as a code owner July 19, 2023 21:35
@reble reble marked this pull request as draft July 26, 2023 13:22
* [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]>
@reble reble marked this pull request as ready for review August 1, 2023 20:37
* 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.
- 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
reble and others added 3 commits August 4, 2023 11:09
* [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.
@sommerlukas
Copy link
Contributor

While revisiting the graph fusion proposal in order to update it to match the merged version of graphs, I noticed a few small issues:

  • The diamond dependency example using buffers violates the buffer limitations and would result in an error if actually implemented. The example would either need to call buffer::set_write_back(false) on the buffers or use the new property assume_buffer_outlives_graph.
  • The example might also be a good place to demonstrate the use of these properties and their effect.
  • Super minor nit: The following code snippet for the depends_on property is missing a { after the namespace declaration:
namespace sycl::ext::oneapi::experimental::property::node
class depends_on {
  public:
    template<typename... NodeTN>
    depends_on(NodeTN... nodes);
};
}

EwanC added a commit to reble/llvm that referenced this pull request Aug 21, 2023
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)
@EwanC EwanC requested a review from a team as a code owner August 22, 2023 10:29
@reble reble requested a review from gmlueck August 23, 2023 14:39
@@ -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`
Copy link
Contributor

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.

Copy link
Contributor

@EwanC EwanC Aug 28, 2023

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?

Copy link
Contributor

@EwanC EwanC Aug 28, 2023

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.

Copy link
Contributor

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

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.

EwanC added a commit to reble/llvm that referenced this pull request Aug 28, 2023
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))
@EwanC EwanC requested a review from gmlueck August 28, 2023 15:12
Copy link
Contributor

@gmlueck gmlueck left a 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.

@steffenlarsen steffenlarsen merged commit e1163ff into intel:sycl Aug 30, 2023
steffenlarsen pushed a commit that referenced this pull request Aug 30, 2023
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants