Skip to content
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

[SYCL][NewOffload][NFC] Add SYCLPostLink library component. #17454

Open
wants to merge 6 commits into
base: sycl
Choose a base branch
from

Conversation

maksimsab
Copy link
Contributor

This change adds a component that should contain all post-link processing (e.g., ESIMD, specialization constants). It is not appropriate to put all this functionality into SYCLLowerIR because there is a dependency in LLVMPasses on SYCLLowerIR. Post-link processing requires capabilities such as linking and pipeline building, which being added to SYCLLowerIR will create a dependancy cycle.

After this change, the dependency diagram will look like the following:

SYCLLowerIR -> Passes -> clang
                -------> opt
                |
                V
                SYCLPostLink -> clang-linker-wrapper
                |-------------> sycl-post-link
                --------------> sycl-jit.

This change adds a component that should contain all post-link processing (e.g., ESIMD,
specialization constants). It is not appropriate to put all this functionality
into SYCLLowerIR because there is a dependency in LLVMPasses on SYCLLowerIR.
Post-link processing requires capabilities such as linking and pipeline
building, which being added to SYCLLowerIR will create a dependancy
cycle.

After this change, the dependency diagram will look like the following:
SYCLLowerIR -> Passes -> clang
                -------> opt
                |
                V
                SYCLPostLink -> clang-linker-wrapper
                |-------------> sycl-post-link
                --------------> sycl-jit.
Copy link
Contributor

@sarnex sarnex left a comment

Choose a reason for hiding this comment

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

lgtm just one nit about the name

@@ -0,0 +1,27 @@
add_llvm_component_library(LLVMSYCLPostLink
Copy link
Contributor

Choose a reason for hiding this comment

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

about the name, to me syclpostlink is a tool, so i would expect files to be in clang/tools, so maybe we could rename it to something else, but i don't have any great ideas, SYCLLowering, SYCLFinalization, idk

Copy link
Contributor

Choose a reason for hiding this comment

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

sycl-post-link's help text reads:

This is a collection of utilities run on device code's LLVM IR before handing off to back-end for further compilation or emitting SPIRV.

... so I think the verb "finalize" would capture that well. SYCLFinalizeIR in analogy to SYCLLowerIR maybe?

I also wouldn't mind keeping the "post-link" moniker. SYCLPostLinkUtils or so would make it clear that this is not a tool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sycl-post-link tool is going to disappear after migration to New Offload Model.
clang/tools is not appropriate since sycl-jit and intel's fortran compiler depend on this functionality.

Copy link
Contributor

@jopperm jopperm left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this!

Copy link
Contributor

@maarquitos14 maarquitos14 left a comment

Choose a reason for hiding this comment

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

LGTM.

@maksimsab
Copy link
Contributor Author

@intel/llvm-gatekeepers Can we merge it?

@aelovikov-intel
Copy link
Contributor

@intel/llvm-gatekeepers Can we merge it?

image

@maksimsab
Copy link
Contributor Author

@aelovikov-intel Please, specify your concern.

If it was a failing ProtexIP, then I didn't find in it's output any clues that it is related to my change.

@aelovikov-intel
Copy link
Contributor

@aelovikov-intel Please, specify your concern.

If it was a failing ProtexIP, then I didn't find in it's output any clues that it is related to my change.

First, you have to let gatekeepers know why you think failures could be ignored. Second, your testing was still in progress when you pinged us (win e2e).

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.

5 participants