-
Notifications
You must be signed in to change notification settings - Fork 788
[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
Conversation
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
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.
Added comments. Please take a look. Thanks
clang/lib/Driver/OffloadBundler.cpp
Outdated
this->TargetID = TargetIdWithFeature; | ||
else | ||
this->TargetID = ""; | ||
if (Components.size() == 5 || Components.size() == 6) { |
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.
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.
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, that makes sense. I had an earlier revision that was similar to that - and it is easier for merging.
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 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; |
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 think we will get warnings for unused variables above.
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.
Thanks - I've updated to move all code into the 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. Thanks
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.
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.
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.