Skip to content

[Driver] Allow for usage of old triple behavior in bundler #17557

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 8 commits into from
Mar 20, 2025

Conversation

mdtoguchi
Copy link
Contributor

@mdtoguchi mdtoguchi commented Mar 20, 2025

Recent upstreamed changes force the triple usage with the bundler to use a 4-field triple value.
The Intel compiler does not take well to these changes. We are not in a position to move our triple usage when bundling and unbundling (not in a breaking change period).

Here, we are updating bundler behavior to allow for the older behavior of accepting triple formats that are 'incomplete'. We can move to using the forced triple value usage when we are ready or if we even need to when we move to the new offloading model.

Updates bundler behavior to allow for the older behavior of accepting
triple formats that are 'incomplete'.  Also performs some cleanup of
tests of removed options and updating packager information to include
'arch=generic' as needed
@mdtoguchi mdtoguchi requested a review from asudarsa March 20, 2025 16:49
Copy link
Contributor

@asudarsa asudarsa left a comment

Choose a reason for hiding this comment

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

Added comments. Please take a look. Thanks

this->TargetID = TargetIdWithFeature;
else
this->TargetID = "";
if (Components.size() == 5 || Components.size() == 6) {
Copy link
Contributor

Choose a reason for hiding this comment

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

One minor nit: May be we can reorg this code as follows (to avoid merge issues)
if (Components.size() < 5) {
...// Do our thing
return;
}
// Add community code as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:) yes, that makes sense. I had an earlier revision that was similar to that - and it is easier for merging.

@mdtoguchi mdtoguchi marked this pull request as ready for review March 20, 2025 22:07
@mdtoguchi mdtoguchi requested review from a team as code owners March 20, 2025 22:07
@mdtoguchi mdtoguchi requested a review from Chenyang-L March 20, 2025 22:07
Copy link
Contributor

@Chenyang-L Chenyang-L left a comment

Choose a reason for hiding this comment

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

lgtm for PD

// FIXME: The Intel Compiler does not currently adhere to the strict
// component size. Consider all checks valid.
// return Components.size() == 5 || Components.size() == 6;
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we will get warnings for unused variables above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks - I've updated to move all code into the comment.

@mdtoguchi mdtoguchi requested a review from asudarsa March 20, 2025 22:34
Copy link
Contributor

@asudarsa asudarsa left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@mdtoguchi mdtoguchi merged commit 674bb8c into sycl-web Mar 20, 2025
@mdtoguchi mdtoguchi deleted the cmplrllvm-66257-drv branch March 20, 2025 23:03
jsji pushed a commit that referenced this pull request Mar 30, 2025
Recent upstreamed changes force the triple usage with the bundler to use
a 4-field triple value.
The Intel compiler does not take well to these changes. We are not in a
position to move our triple usage when bundling and unbundling (not in a
breaking change period).

Here, we are updating bundler behavior to allow for the older behavior
of accepting triple formats that are 'incomplete'. We can move to using
the forced triple value usage when we are ready or if we even need to
when we move to the new offloading model.
jsji pushed a commit that referenced this pull request Apr 1, 2025
Recent upstreamed changes force the triple usage with the bundler to use
a 4-field triple value.
The Intel compiler does not take well to these changes. We are not in a
position to move our triple usage when bundling and unbundling (not in a
breaking change period).

Here, we are updating bundler behavior to allow for the older behavior
of accepting triple formats that are 'incomplete'. We can move to using
the forced triple value usage when we are ready or if we even need to
when we move to the new offloading model.
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.

4 participants