-
Notifications
You must be signed in to change notification settings - Fork 796
[SYCL][NFC] Rename support dynamic linking #15258
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
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.
based on the description of the help option, the name allow-device-image-dependencies is more clear to me, allow-device-dependencies isn't too clear.
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 clang level option to control this behavior is -fsycl-allow-device-dependencies. Should we be consistent with the clang option or make the option name clearer?
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 clang option name is unclear to me too, but if we already went through the options working group and they approved it then it's fine to match it here.
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.
Should we be consistent with the clang option or make the option name clearer?
I don't think that we have to 100% match here. device-dependencies does not look entirely clear to me. I won't strongly object, but my preference is to have clearer name, even if it is different from clang one.
I would rather talk to options wg once again to change public name as well to device-image-dependencies or device-code-dependencies
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'll ask the options WG.
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.
Should we try to change the option name for 2025.0, since it is a new option, or add a synonymous option name -fsycl-allow-device-image-dependencies for 2025.1 ?
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.
Definitely too late for 2025.0, we would have to deprecate and add the new option moving forward with 2025.1
asudarsa
left a 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.
Looks good except for option name. Will approve once name is updated. Thanks much
…-device-dependencies Signed-off-by: Lu, John <john.lu@intel.com>
Signed-off-by: Lu, John <john.lu@intel.com>
Changed to -allow-device-image-dependencies in sycl-post-link. Clang option will be changed to -fsycl-allow-device-image-dependencies when options WG approves. |
asudarsa
left a 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
Rename sycl-post-link option "-support-dynamic-linking" to "-allow-device-image-dependencies". This is consistent with the clang option that is tied to this sycl-post-link option, and is also consistent with what is being done inside sycl-post-link.