Skip to content

[SYCL][thinLTO] Seperate module properties and symbol table generation into IR-based analysis #14220

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 14 commits into from
Jun 28, 2024

Conversation

sarnex
Copy link
Contributor

@sarnex sarnex commented Jun 18, 2024

Based on feedback from #14197, I seperated out the code that generates the module properties and symbol table into separate functions that can be called by anyone, and just looks at the IR and entry points.

For now, we still call it inside sycl-post-link because we still support the old offloading model, but once we drop support for that we can drop this responsibility from sycl-post-link and only compute it inside clang-linker-wrapper, both for normal compilation and thinLTO.

In a (hopefully soon) future PR I plan to call these functions from clang-linker-wrapper when compiling for thinLTO, which we need because we will split early.

Most of this change should be NFC(I). The expected changes are:

  1. New option to sycl-post-link to generate the properties file
  2. Driver change to NOT pass the option from 1) in thinLTO mode
  3. Two minor chages in logic from properties generation, I've called these out inline.

!F.getName().starts_with("__itt"))
SeenSYCLFunction = true;
}
if (SeenESIMDFunction && !SeenSYCLFunction)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This logic is new. Previously we used MD.isESIMD()

// 'if' below essentially preserves the behavior (presumably mistakenly)
// implemented in intel/llvm#8763: ignore 'optLevel' property for images which
// were produced my merge after ESIMD split
if (!SeenESIMDFunction || !SeenSYCLFunction) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This logic is new. Previously we used MD.getEntryPointGroup().Props.HasESIMD != module_split::SyclEsimdSplitStatus::SYCL_AND_ESIMD

@sarnex sarnex marked this pull request as ready for review June 21, 2024 14:31
@sarnex sarnex requested review from a team as code owners June 21, 2024 14:31
Copy link
Contributor

@bader bader 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 minor comments.

Signed-off-by: Sarnie, Nick <[email protected]>
@sarnex sarnex requested review from a team as code owners June 21, 2024 20:04
@sarnex sarnex requested a review from cperkinsintel June 21, 2024 20:04
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

Thanks a lot for making this improvement!

Signed-off-by: Sarnie, Nick <[email protected]>
Signed-off-by: Sarnie, Nick <[email protected]>
@sarnex sarnex temporarily deployed to WindowsCILock June 21, 2024 22:28 — with GitHub Actions Inactive
Copy link
Contributor

@PietroGhg PietroGhg left a comment

Choose a reason for hiding this comment

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

Change in the Native CPU test LGTM, thank you

Signed-off-by: Sarnie, Nick <[email protected]>
@sarnex sarnex temporarily deployed to WindowsCILock June 25, 2024 18:34 — with GitHub Actions Inactive
@sarnex sarnex temporarily deployed to WindowsCILock June 25, 2024 19:17 — with GitHub Actions Inactive
@sarnex sarnex requested review from fineg74 June 25, 2024 19:33
@sarnex sarnex temporarily deployed to WindowsCILock June 26, 2024 21:03 — with GitHub Actions Inactive
@sarnex sarnex temporarily deployed to WindowsCILock June 27, 2024 01:07 — with GitHub Actions Inactive
@sarnex
Copy link
Contributor Author

sarnex commented Jun 27, 2024

@intel/llvm-reviewers-runtime Does someone mind taking a look? Only relevant change should be adding the new -properties flag to sycl-post-link calls in tests. Thanks.

Signed-off-by: Sarnie, Nick <[email protected]>
Copy link
Contributor

@maksimsab maksimsab left a comment

Choose a reason for hiding this comment

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

LGTM.

nit: I would rather generate properties and symbols unconditionally since it doesn't throw errors and it is not really costly. Anyway, this is what we are going to do in clang-linker-wrapper.

@sarnex
Copy link
Contributor Author

sarnex commented Jun 28, 2024

@maksimsab Thank you for the review!! I agree it's not costly, but it does incur extra file IO if we write them to disk as we do today (which admittedly should not really be noticeable). The main idea here is that we can seperate out the splitting and props/symtable generation as seperate operations, which is important for thinLTO.

@sarnex sarnex temporarily deployed to WindowsCILock June 28, 2024 15:10 — with GitHub Actions Inactive
@sarnex sarnex merged commit 94f6b2f into intel:sycl Jun 28, 2024
14 checks passed
@sarnex
Copy link
Contributor Author

sarnex commented Jun 28, 2024

@AlexeySachkov I know you're OOO but I'd like your review on this, I merged it now because there are a lot of changes and I had to rebase/merge almost daily. I'm happy to address any comments in a follow-up PR.

calebwat pushed a commit that referenced this pull request Jul 1, 2024
…n into IR-based analysis (#14220)

Based on feedback from #14197, I
seperated out the code that generates the module properties and symbol
table into separate functions that can be called by anyone, and just
looks at the IR and entry points.

For now, we still call it inside `sycl-post-link` because we still
support the old offloading model, but once we drop support for that we
can drop this responsibility from sycl-post-link and only compute it
inside `clang-linker-wrapper`, both for normal compilation and thinLTO.

In a (hopefully soon) future PR I plan to call these functions from
`clang-linker-wrapper` when compiling for thinLTO, which we need because
we will split early.

Most of this change should be NFC(I). The expected changes are:

1) New option to sycl-post-link to generate the properties file
2) Driver change to NOT pass the option from 1) in thinLTO mode
3) Two minor chages in logic from properties generation, I've called
these out inline.

---------

Signed-off-by: Sarnie, Nick <[email protected]>
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.

8 participants