Skip to content

Task sequence revert #14359

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

Merged
merged 7 commits into from
Jul 11, 2024
Merged

Task sequence revert #14359

merged 7 commits into from
Jul 11, 2024

Conversation

aejjehint
Copy link
Contributor

@aejjehint aejjehint commented Jun 28, 2024

This reverts PR #12453 and #13080

The original PR introducing task sequence was checked in prematurely as the backend support isn't in yet. When the backend support is in we will re-submit this change.

@aejjehint aejjehint requested review from a team as code owners June 28, 2024 19:55
Copy link
Contributor

@AlexeySachkov AlexeySachkov left a 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 understand motivation for the revert - did we decide to drop the feature, or did it cause some regressions which are hard to debug?

@GarveyJoe
Copy link
Contributor

I agree with Alexey, please include in the description the reasoning for the revert: i.e.:

"This change was checked in prematurely as the backend support isn't in yet. When the backend support is in we will re-submit this change."

Copy link
Contributor

@GarveyJoe GarveyJoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commented

@aejjehint
Copy link
Contributor Author

@AlexeySachkov @GarveyJoe I added to the description the reason behind the revert.

@aejjehint aejjehint force-pushed the task_sequence_revert branch from f1b3ba4 to 956da95 Compare July 9, 2024 21:33
@steffenlarsen steffenlarsen merged commit e40283b into intel:sycl Jul 11, 2024
13 checks passed
// __SYCL_ASPECT(ext_intel_fpga_task_sequence__, 62)
#define __SYCL_ANY_DEVICE_HAS_ext_intel_fpga_task_sequence__ 0
#endif

#ifndef __SYCL_ANY_DEVICE_HAS_ext_oneapi_limited_graph__
// __SYCL_ASPECT(ext_oneapi_limited_graph, 63)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the following defines should all have been updated, similar to ALL_DEVICE_HAS.

The ID specified for ext_oneapi_limited_graph now does not match the ID defined in aspects.def anymore. The same applies for all following defines.

@aejjehint Can you please fix this in a follow-up PR?

steffenlarsen pushed a commit that referenced this pull request Jul 22, 2024
This PR reverts the aspect numbering that was modified in PR #14359 to
avoid breaking existing code.
againull pushed a commit that referenced this pull request Jul 26, 2024
This change re-apply the task sequence changes from PR #12453 that were
reverted in #14359 now that the spec has been approved and the back-end
implementation is ready. The spec is also moved from proposed to
experimental.

---------

Co-authored-by: bowenxue-intel <[email protected]>
Co-authored-by: Steffen Larsen <[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.

8 participants