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 definition MDSPAN_USE_PAREN_OPERATOR=1 from DDC target #677

Merged
merged 4 commits into from
Nov 5, 2024

Conversation

tpadioleau
Copy link
Member

@tpadioleau tpadioleau commented Nov 2, 2024

DDC should not force dependent projects to use MDSPAN_USE_PAREN_OPERATOR=1. Moreover this variable only makes sense in the Kokkos implementation of mdspan. A compiler provided implementation will likely not provide this variable neither the function call operator, see cppref.

In this PR, DDC adapts to the mdspan implementation, defaulting on using the bracket operator.

Note that there is a bug with llvm 15/16, see the reproducer on godbolt. It has been reported on the mdspan repo, see here.

The two first commits provide alternative implementations that correctly call mdspan either with:

  • a new function
  • a new preprocessor macro

@tpadioleau tpadioleau self-assigned this Nov 2, 2024
@tpadioleau tpadioleau linked an issue Nov 2, 2024 that may be closed by this pull request
@tpadioleau tpadioleau force-pushed the 676-do-not-force-mdspan_use_paren_operator=1 branch 2 times, most recently from 5f4dd1f to 6a1439e Compare November 2, 2024 11:21
@tpadioleau tpadioleau force-pushed the 676-do-not-force-mdspan_use_paren_operator=1 branch from 6a1439e to bb724ee Compare November 2, 2024 11:42
@tpadioleau tpadioleau marked this pull request as ready for review November 4, 2024 16:40
Copy link
Member

@yasahi-hpc yasahi-hpc left a comment

Choose a reason for hiding this comment

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

Almost LGTM.
I have a question about the logic of macro

.github/workflows/tests.yml Show resolved Hide resolved
include/ddc/detail/kokkos.hpp Outdated Show resolved Hide resolved
yasahi-hpc
yasahi-hpc previously approved these changes Nov 5, 2024
@tpadioleau
Copy link
Member Author

Any preference between introducing a macro or a function ?

@yasahi-hpc
Copy link
Member

Any preference between introducing a macro or a function ?

I think macro is good enough.

Copy link
Member

@yasahi-hpc yasahi-hpc left a comment

Choose a reason for hiding this comment

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

LGTM

@tpadioleau tpadioleau merged commit 4bae03b into main Nov 5, 2024
55 checks passed
@tpadioleau tpadioleau deleted the 676-do-not-force-mdspan_use_paren_operator=1 branch November 5, 2024 16:13
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.

Do not force MDSPAN_USE_PAREN_OPERATOR=1
2 participants