[New offload driver][sycl-post-link] Move sycl-post-link target specific options generation to linker wrapper #14101
Conversation
…specific options generation to linker wrapper Signed-off-by: Arvind Sudarsanam <[email protected]>
Signed-off-by: Arvind Sudarsanam <[email protected]>
| CmdArgs.end()); | ||
| }; | ||
|
|
||
| // specialization constants processing. |
There was a problem hiding this comment.
I wonder if we can limit the cases where we have to remove or replace args as much as possible. In the ideal world, the driver would only pass these options as ClangLinkerWrapper options (and not any tool options) and ClangLinkerWrapper would know how to create the tool options based on the ClangLinkerWrapper options. Having the driver set tool options but then replacing/removing them in ClangLinkerWrapper seems like something we should try to minimize. It looks like we are stopping the driver from passing some options when the new offload driver is on, but I wonder if we plan to reduce the things this function is doing even more in the future.
There was a problem hiding this comment.
Thanks @sarnex. This is a good observation. I did spend some cycles trying to think of a better way. Following are the factors behind my current design. I will take -emit-only-kernel-as-entry-points as an example.
This option to sycl-post-link is controlled by two factors:
- The -fsycl option passed by the user. This information is only available in the driver.
- Target triple of the device image that is being post-linked. This information is embedded in the device image and is parsed by the linker wrapper just before sycl-post-link. Again, this information is available only inside the linker wrapper.
We already have many sycl-specific options being passed into the linker wrapper. I think the trade-off here is between number of sycl-specific options passed into the linker wrapper versus adding logic to update options inside the '--sycl-post-link-options' based on the current target. I have gone with adding logic to update options.
May be we can document this and try to revisit at a later time if agreeable?
Thanks
Sincerely
There was a problem hiding this comment.
Thanks for the explanation. Let's assume for discussion clang-linker-wrapper has a way to get the offload kind and the option value (so it knows the current image is SYCL and it knows emit-only-kernel-as-entry-points was passed by the user to clang++).
So I guess the discussion is, should these options be first-class to clang-linker-wrapper (meaning we need to add a new clang-linker-wrapper option for all of the SYCL options that effect device linking) or should they be opaque to clang-linker-wrapper (meaning they are blindly passed as a string from the driver and clang-linker-wrapper will have to do string manipulation on those options). My intuition is the first-class option is cleaner, but I'd like to get opinions from others (@bader @AlexeySachkov etc).
This discussion doesn't necessarily have to block this PR but I think we should make a design decision at some point.
There was a problem hiding this comment.
I do not have strong opinion on either option. Surely, +1 on the fact that passing new options is a cleaner solution. I will create a tracker for adding new linker wrapper options for such cases. Hopefully, it need not block this PR.
Thanks
There was a problem hiding this comment.
I agree with @sarnex. clang-linker-wrapper doesn't seem like the right tool to update sycl-post-link options based on LLVM target triple. sycl-post-link tools itself should be able do that. Why can't we move this logic there?
There was a problem hiding this comment.
That sounds good to me, but I still would like to minimize string manipulation and rather combine triple information with option value information to decide behavior.
Signed-off-by: Arvind Sudarsanam <[email protected]>
Signed-off-by: Arvind Sudarsanam <[email protected]>
Signed-off-by: Arvind Sudarsanam <[email protected]>
Signed-off-by: Arvind Sudarsanam <[email protected]>
|
@intel/llvm-gatekeepers This is ready for merging. Can you please help? Thanks |
There was a problem hiding this comment.
Driver/sycl-offload-new-driver.c is failing in post-commit after #14091. I am blocking this PR until the patch has either been reverted or the test failure has been addressed.
|
Testing was done prior to merging the breaking PR, so this should be good to merge. |
…fic options generation to linker wrapper (intel#14101) There are some sycl-post-link options that are dependent on target triples and some options that are dependent on user options and some options that are dependent on both. In this change, we set up all the sycl-post-link options in the driver and then pass them to the clang-linker-wrapper tool. Before calling sycl-post-link functionality, the clang-linker-wrapper will update these options based on the target triple of the image being processed. Several tests have been updated as ordering of options have changed. Thanks --------- Signed-off-by: Arvind Sudarsanam <[email protected]>
…ted (#14177) In an earlier PR (#14101), we added support to generate sycl-post-link options (that partly depend on target triple) in clang Driver and then update the options using target triple information in the clang-linker-wrapper. This PR cleans up that implementation by fully generating these options inside the clang-linker-wrapper. This requires some more user-specified options to be passed into the clang-linker-wrapper. Thanks ------ Signed-off-by: Arvind Sudarsanam <[email protected]>
There are some sycl-post-link options that are dependent on target triples and some options that are dependent on user options and some options that are dependent on both.
In this change, we set up all the sycl-post-link options in the driver and then pass them to the clang-linker-wrapper tool. Before calling sycl-post-link functionality, the clang-linker-wrapper will update these options based on the target triple of the image being processed.
Several tests have been updated as ordering of options have changed.
Thanks