Skip to content

<mdspan>: Fix layout_stride::mapping<E>::is_exhaustive() #5477

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

Merged
merged 8 commits into from
May 10, 2025

Conversation

JMazurkiewicz
Copy link
Contributor

Currently, our implementation of layout_stride::mapping<E>::is_exhaustive() gives incorrect result for some "unusual" extents. This PR attempts to fix it by adding a new way to compute an answer to the problem described in [mdspan.layout.stride.obs]/5.2 and extra return paths for simple cases:

  • When E::rank() == 1, is_exhaustive() returns true if and only if stride(0) == 1.
  • When E::rank() == 2, is_exhaustive() returns true if and only if one of the strides is equal to 1 and the other stride is equal to extent corresponding to the previous stride.
  • When two or more extents are equal to 0, then is_exhaustive() always returns false.
  • When there are no extents equal to 0 or 1 (these are "unusual" extents) this functions uses current method of comparing required_span_size() to fwd-prod-of-extents(rank) (extracted to separate function named _Is_exhaustive_common_case).
  • Otherwise, _Is_exhaustive_special_case computes correct answer by finding a permutation of (stride, extent) pairs described in [mdspan.layout.stride.obs]/5.2. If such permutation does not exist this function returns false.
    • Sometimes, when one or more extents are equal to 1, mapping might be exhaustive, but this function (because of the way it is described) should return false. For example:
    extents = [4, 1]
    strides = [1, 5]
    required_span_size = 1 + (4 - 1) * 1 + (1 - 1) * 5 = 4
    fwd-prod-of-extents = 4 * 1 = 4 // equal to required_span_size, `_Is_exhaustive_common_case` would return true
    m(0, 0) = 0
    m(1, 0) = 1
    m(2, 0) = 2
    m(3, 0) = 3
    This mapping is clearly exhaustive according to [mdspan.layout.reqmts]/16, but not per [mdspan.layout.stride.obs]/5.2.

libc++ test fails now due to bogus test - mapping tested here is clearly not exhaustive (at least one of the strides should be equal to 1). This also indicates that their implementation is incorrect.

@JMazurkiewicz JMazurkiewicz requested a review from a team as a code owner May 7, 2025 17:58
@github-project-automation github-project-automation bot moved this to Initial Review in STL Code Reviews May 7, 2025
@StephanTLavavej StephanTLavavej added bug Something isn't working mdspan C++23 mdspan labels May 7, 2025
@StephanTLavavej StephanTLavavej self-assigned this May 7, 2025
@StephanTLavavej
Copy link
Member

Thanks! 😻 I pushed minor changes, please meow if I messed something up.

@StephanTLavavej StephanTLavavej removed their assignment May 9, 2025
@StephanTLavavej StephanTLavavej moved this from Initial Review to Ready To Merge in STL Code Reviews May 9, 2025
@StephanTLavavej StephanTLavavej moved this from Ready To Merge to Merging in STL Code Reviews May 9, 2025
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

StephanTLavavej added a commit to StephanTLavavej/STL that referenced this pull request May 9, 2025
@StephanTLavavej StephanTLavavej merged commit 4e5eb85 into microsoft:main May 10, 2025
39 checks passed
@github-project-automation github-project-automation bot moved this from Merging to Done in STL Code Reviews May 10, 2025
@StephanTLavavej
Copy link
Member

Thanks for investigating this libcxx skip! 🛠️ 🕵️ 😻

@JMazurkiewicz JMazurkiewicz deleted the mdspan/is_exhaustive branch May 12, 2025 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working mdspan C++23 mdspan
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants