Skip to content

Conversation

@carbotaniuman
Copy link
Contributor

This removes several bypasses put in place for AdaptiveCpp. I'm not exactly sure what the contribution process is or how to edit the filter in the ci folder, so please let me know. I think these are best reviewed commit-by-commit.

@carbotaniuman carbotaniuman requested a review from a team as a code owner August 21, 2025 23:25
@CLAassistant
Copy link

CLAassistant commented Aug 21, 2025

CLA assistant check
All committers have signed the CLA.

@bader
Copy link
Contributor

bader commented Aug 21, 2025

I'm not exactly sure what the contribution process is or how to edit the filter in the ci folder, so please let me know.

I hope https://github.com/KhronosGroup/SYCL-CTS/blob/main/docs/README.adoc answers all your questions. Let us know if it's not the case.

@carbotaniuman
Copy link
Contributor Author

I'm reading the document here and it is quite useful thanks. I have a question - it doesn't look like I can update the ACPP version myself as I don't have repo perms, I think Aksel would need to do that?

@bader
Copy link
Contributor

bader commented Aug 22, 2025

I have a question - it doesn't look like I can update the ACPP version myself as I don't have repo perms, I think Aksel would need to do that?

Anyone with write access to the repository can update ACPP version. If you specify the SHA of the ACPP commit you want to use, I can try to create a docker image with the ACPP compiler.

@carbotaniuman
Copy link
Contributor Author

I think I can do latest master - 65e010622e37b25d75b70d41776ebb2685b2e4a8.

@bader
Copy link
Contributor

bader commented Aug 22, 2025

I think I can do latest master - 65e010622e37b25d75b70d41776ebb2685b2e4a8.

I've created #1138. If CI passes, I'll merge tomorrow. After that, you will need to update your branch with these changes (i.e. merge main to carbotaniuman:update-acpp-bypasses) to test with the updated AdaptiveCPP.

@bader
Copy link
Contributor

bader commented Aug 22, 2025

@carbotaniuman, I've updated the branch to test your changes and latest AdaptiveCPP emits errors. Please, take a look at the logs.

@tomdeakin
Copy link
Contributor

tomdeakin commented Sep 4, 2025

@carbotaniuman - please remember to sign the CLA. Thanks for your contributions.

@keryell
Copy link
Member

keryell commented Oct 16, 2025

@carbotaniuman Any news about this?

@carbotaniuman
Copy link
Contributor Author

Sorry, there were some issues revealed with my changes that I had to fix ACPP for, will update this and push a new version.

@carbotaniuman
Copy link
Contributor Author

Ok, pushed a new version, can I have acpp bumped to fcd26a61b600cd4b653724e59e72d4f7d1da259c to see if it works?

@bader
Copy link
Contributor

bader commented Oct 27, 2025

Ok, pushed a new version, can I have acpp bumped to fcd26a61b600cd4b653724e59e72d4f7d1da259c to see if it works?

I've created #1151. I'll merge it when CI passes.

@bader
Copy link
Contributor

bader commented Oct 27, 2025

Ok, pushed a new version, can I have acpp bumped to fcd26a61b600cd4b653724e59e72d4f7d1da259c to see if it works?

@carbotaniuman, unfortunately, the accp built from fcd26a61b600cd4b653724e59e72d4f7d1da259c still can't compile all enabled tests. In addition to that, there are some formatting issues with the pull request. Could you take a look please?

This removes some ifdefs that are no longer needed while adding more
fine-grained defines.
@bader
Copy link
Contributor

bader commented Oct 27, 2025

@carbotaniuman, I would appreciate, if you use "merge" instead of "rebase" strategy to update your branch.
NOTE: recent rebase doesn't address formatting issues as well as compilation failures with acpp.

@bader bader requested a review from a team October 29, 2025 05:20
@bader bader requested a review from illuhad October 30, 2025 15:07
Copy link
Contributor

@illuhad illuhad left a comment

Choose a reason for hiding this comment

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

LGTM

@tomdeakin
Copy link
Contributor

WG approved to merge.

@tomdeakin tomdeakin merged commit 52e0d8c into KhronosGroup:main Oct 30, 2025
9 checks passed
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.

6 participants