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.

Sign up for GitHub

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

Jump to bottom

Copy / MR Improvements, main branch (2023.11.10.) #247

Merged
merged 4 commits into from
Nov 12, 2023
Merged

Copy / MR Improvements, main branch (2023.11.10.) #247

merged 4 commits into from
Nov 12, 2023

Conversation

krasznaa
Copy link
Member

@krasznaa krasznaa commented Nov 10, 2023 •
edited
Loading

I ran into a few things while trying to make things work inside of WSL2 with oneAPI's NVIDIA backend.

  • For some reason GCC 9 in Ubuntu 20.04 in WSL2 has an issue with the symbols of the virtual functions on the copy types not being exported. 😕 Very strange given that we haven't seen issues with this in any other place. (It's a question of whether the compiler is smart enough to know at compile time which function it would have to call, relying on override and final declarations.)
  • I kept seeing a crash in one of the SYCL tests that just didn't give me a usable/understandable backtrace. 😦 In the end it turned out to be an issue with asynchronous memory copies into SYCL shared memory.
    • But while hunting for the issue, I ended up making copies into host jagged vectors a little smarter. (Thinking that this may be the issue behind the crash.) Ensuring that the output jagged vector of a copy would use the same memory resource for all of its std::vector components.

While at it, I also made sure that all virtual functions on the vecmem::copy and memory resource classes would use the same set of keywords. Adding virtual and final in places where they were missing before. (The first one is just a stylistic thing, but the added final keywords may help slightly in some cases.)

@krasznaa krasznaa force-pushed the CopyImprovements-main-20231110 branch from a5b0502 to f6bb77b Compare November 10, 2023 14:17
@krasznaa krasznaa marked this pull request as ready for review November 11, 2023 10:37
@krasznaa
Explicitly exporting the copy virtual functions.
e829178 
With certain compilers this ends up being needed.
@krasznaa
Made vecmem::copy resize jagged vectors correctly.
ce7f2c7 
Making sure that the inner vectors would use the same
memory resource that was assigned to the outer vector.
@krasznaa
Switch filter test to using a host memory resource.
97143c8 
On WSL2 asynchronous copying into shared memory (with an
NVIDIA backend) does not work for some reason. Leading to
a very weird crash.
@krasznaa
Synchronizing keywords on copy and resource classes.
349acb5 
Making sure that they all use the same set of keywords for
declaring their virtual functions, in all of the libraries.
@krasznaa krasznaa force-pushed the CopyImprovements-main-20231110 branch from cdf5e08 to 349acb5 Compare November 12, 2023 10:10
@krasznaa krasznaa changed the title Copy Improvements, main branch (2023.11.10.) Copy / MR Improvements, main branch (2023.11.10.) Nov 12, 2023
@krasznaa krasznaa merged commit ec34d93 into acts-project:main Nov 12, 2023
@krasznaa krasznaa deleted the CopyImprovements-main-20231110 branch November 12, 2023 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@krasznaa