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

refactor: VecMem-ification of the Sycl Seedfinder Plugin #998

Merged

Conversation

konradkusiak97
Copy link

The current implementation of the Sycl Seedfinder uses pointers to manage memory allocations on the host and on the device. This PR changes this approach by implementing VecMem for memory management.

Under the supervision of @krasznaa, I updated the code so that it works fully on VecMem objects. A short summary of the changes is the following:

  • Inside the CreateSeedsForGroupSycl() all the host and device objects are now vecmem::vector(s) or vecmem::data::vector_buffer(s). They are received in the kernels as vecmem::data::vector_view(s) and manipulated there as vecmem::device_vector(s).
  • The vecmem::jagged_vector was used for the Duplet finding and as a result, the atomic count was removed from the DupletSearch kernels.
  • The Sycl seedfinder is now able to accept device and host memory resources or shared memory resource. To control this different behaviour, a bunch of if statements and unique pointers were written inside the CreateSeedsForGroupSycl.cpp.
  • The TripletSearch and TripletFinding kernels were moved to separate files and rewritten as function objects.

@paulgessinger
Copy link
Member

So one thing that we would have to figure out is how to handle the vecmem dependency. We had decided a long(ish) while ago that we would bundle and ship the dependency code. We do this for a number of libraries. However, we recently introduced convenience helpers similar to this PR's ActsVecMem.cmake for boost and eigen.

I'm a bit torn, honestly, how we should go about this. Ideally I would think we should handle this as consistently as possible.

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

I know, I know... I should've looked in this detail already earlier... 😦 My comments are really just surface level though... 😉

@krasznaa
Copy link
Member

krasznaa commented Sep 21, 2021

I'm a bit torn, honestly, how we should go about this. Ideally I would think we should handle this as consistently as possible.

I personally like the ActsVecMem.cmake style approach. (Shocker, I know...) I would try to make the projects use each other in this sort of way. Though Konrad actually already has a fix for that file, which I forgot to put a comment on...

@paulgessinger
Copy link
Member

Indeed, shocker! Maybe we can discuss how to go about handling thirdparty modules in next week's ACTS core meeting?

@krasznaa
Copy link
Member

Apart from a single comment (removing all changes from ActsCompilerOptions.cmake), I'm fine with the rest of the PR. Could the project managers (@paulgessinger ?) enable the CI on the PR? It would be good to get this in, so that we can continue with further developments. Without having to worry about compatibility with the open PR.

Konrad Kusiak and others added 3 commits September 27, 2021 11:48
Whether Acts would build VecMem itself is now controlled by the
ACTS_USE_SYSTEM_VECMEM configuration option, and the build configuration
has been moved to the thirdparty/vecmem subdirectory.

The build of VecMem is always set up, however its libraries are only
built if some other part of Acts is actually using them.
At the same time removing the "optnone" flag from the main function
of the seed finding.
@codecov
Copy link

codecov bot commented Sep 29, 2021

Codecov Report

Merging #998 (6980485) into main (adfffb6) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #998   +/-   ##
=======================================
  Coverage   48.60%   48.60%           
=======================================
  Files         337      337           
  Lines       17329    17329           
  Branches     8191     8191           
=======================================
  Hits         8423     8423           
  Misses       3165     3165           
  Partials     5741     5741           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adfffb6...6980485. Read the comment docs.

@krasznaa krasznaa added this to the next milestone Sep 29, 2021
At the same time removed the EXCLUDE_FROM_ALL flag from add_subdirectory(...),
to make sure that the VecMem libraries would be installed together with Acts
when Acts builds VecMem.
Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

I believe this is as good as it's going to get...

@paulgessinger, I had to realise that the way that we've been building VecMem as part of Acts (and as part of all of the other projects...) so far was quite wrong. Since if we use add_subdirectory(...) with the EXCLUDE_FROM_ALL flag, the code from the subdirectory would not become part of the project's installation. I.e. the main VecMem libraries are not installed together with the project (with make install). Since all of the other projects (detray and traccc) we're only using "from their build directories" so far, this never showed up. 🤔

So now I ditched the usage of EXCLUDE_FROM_ALL, and switched to using FetchContent_MakeAvailable(...) instead. We will need to re-think how the R&D projects use each other as well...

@konradkusiak97, note that I just now found that the __attribute__(noopt) setting was still active in the PR. I now removed that. It may actually speed up some of your tests! 😉

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

4 participants