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

Remove AdaptiveCpp workaround in dpl_shim.h to allow for automatic prefetch optimization #177

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

illuhad
Copy link
Contributor

@illuhad illuhad commented Dec 1, 2023

To my knowledge, the AdaptiveCpp-specific workaround in dpl_shim.h is no longer necessary as the linking issue was fixed some time ago.

Additionally, this workaround prevents a new optmization in AdaptiveCpp stdpar from taking place: AdaptiveCpp stdpar can now emit automatic data prefetches. But this only works, if the data is managed by the stdpar runtime, which tracks pointers it knows. If sycl::malloc_shared is used directly, the stdpar optimizations do not recognize the pointer and won't apply the optimization.
(the reasoning being that if someone calls SYCL functionality directly, they might want to exert more control and emit prefetches or mem_advise operations by themselves).

Other projects which also use this header like cloverleaf are also affected. Will create a PR there too.

@tom91136
Copy link
Member

tom91136 commented Dec 4, 2023

Thanks, looks good. For the record, do I need a specific commit or branch for prefetch to work or is the latest commit OK?

@tomdeakin
Copy link
Contributor

Please update the CHANGELOG, and consider if this needs to go to main or develop. I'll revert the commit in the mean time.

@tomdeakin
Copy link
Contributor

tomdeakin commented Dec 4, 2023

@illuhad @tom91136 Please update the PR to pull into develop with an updated CHANGELOG.
Edit; I think GitHub needs that to be an entirely new PR...

@illuhad
Copy link
Contributor Author

illuhad commented Dec 4, 2023

Thanks, looks good. For the record, do I need a specific commit or branch for prefetch to work or is the latest commit OK?

It's on develop. But I'm still working on optimizing. I think for your software it's all good, but e.g. LULESH is submitting a ton of small kernels where the additional overhead is observable for small problems.

@illuhad @tom91136 Please update the PR to pull into develop with an updated CHANGELOG.
Edit; I think GitHub needs that to be an entirely new PR...

Ok, I can do that. What is your release schedule? The workaround on current main is only relevant for an earlier prerelease version of AdaptiveCpp stdpar, and is already unnecessary for AdaptiveCpp 23.10.0 release. And it has detrimental effect now. So I'd be much in favor if the main branch could be fixed sooner rather than later. Especially while main is set as main branch in the repository that people pull from.

@illuhad
Copy link
Contributor Author

illuhad commented Dec 4, 2023

The changes in CHANGELOG are grouped by version. Am I supposed to create a new section for the next version? If so, what is the next version going to be?
I wonder if there's some documentation on the process missing, if we really want all changes to develop to be tracked in the CHANGELOG. (For releases, we could always just let github autogenerate a changelog, and paste that into CHANGELOG).

@tomdeakin
Copy link
Contributor

Re Release Schedule: we haven't done one for a while, but I think we're collecting lots of minor things that would warrant a 5.1 release soon.

I normally just add an ## Unreleased date stamp at the top, but looks like I forgot to do it this time, apologies. But if we think GitHub can do a better CHANGELOG that a human, we could compare a manual CHANGELOG to that when we release 5.1 and decide on what to do for the future.

@illuhad
Copy link
Contributor Author

illuhad commented Dec 4, 2023

Re Release Schedule: we haven't done one for a while, but I think we're collecting lots of minor things that would warrant a 5.1 release soon.

Ok, that's great, thanks :)

I normally just add an ## Unreleased date stamp at the top, but looks like I forgot to do it this time, apologies. But if we think GitHub can do a better CHANGELOG that a human, we could compare a manual CHANGELOG to that when we release 5.1 and decide on what to do for the future.

The Unreleased section was on develop, but not on main, so that one was on me :)
What I did notice though when rebasing is that this file is probably going to cause frequent merge conflicts, since every PR will want to modify it. It might be fine while PR rate of this project is on the low side, but I'm not sure what will happen should that increase.

The github-generated changelog depends on the title of PRs, so before merging it would be good to check the PR titles and potentially rename them if they are not precise enough. On the plus side, github changelogs also handle correct attribution of contributions.
As an example, the changelog here was auto-generated: https://github.com/AdaptiveCpp/AdaptiveCpp/releases/tag/v0.9.4

pranav-sivaraman pushed a commit to hpcgroup/BabelStream that referenced this pull request Dec 7, 2023
pranav-sivaraman pushed a commit to hpcgroup/BabelStream that referenced this pull request Dec 7, 2023
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.

None yet

3 participants