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

Transfer span from Singe #47

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Transfer span from Singe #47

wants to merge 7 commits into from

Conversation

BrendanKKrueger
Copy link
Collaborator

PR Summary

An implementation of std::span that works on both the CPU and the GPU.

PR Checklist

  • Any changes to code are appropriately documented.
  • Code is formatted.
  • Install test passes.
  • Docs build.
  • If preparing for a new release, update the version in cmake.

@BrendanKKrueger BrendanKKrueger added the enhancement New feature or request label Aug 26, 2024
@BrendanKKrueger BrendanKKrueger self-assigned this Aug 26, 2024
@BrendanKKrueger BrendanKKrueger marked this pull request as ready for review August 29, 2024 20:42
Copy link
Collaborator

@mauneyc-LANL mauneyc-LANL left a comment

Choose a reason for hiding this comment

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

I have a few changes and/or points of discussion.

In general, I'm okay with this as a placeholder awaiting the move to C++20. If it's of interest, I have some code with a more "compliant" span (the code is unfinished, but span is finished).

There is an general question with the introduction of span; the next obvious step is mdspan, but this would basically supersede PortableMDArray (and, in my opinion, this should happen). Is this the goal?

ports-of-call/span.hpp Outdated Show resolved Hide resolved
ports-of-call/span.hpp Show resolved Hide resolved
ports-of-call/span.hpp Outdated Show resolved Hide resolved
ports-of-call/span.hpp Outdated Show resolved Hide resolved
@BrendanKKrueger
Copy link
Collaborator Author

I have a few changes and/or points of discussion.

In general, I'm okay with this as a placeholder awaiting the move to C++20. If it's of interest, I have some code with a more "compliant" span (the code is unfinished, but span is finished).

There is an general question with the introduction of span; the next obvious step is mdspan, but this would basically supersede PortableMDArray (and, in my opinion, this should happen). Is this the goal?

  • span: This version of span (actually called span_like in Singe) was specifically tailored to only implement the bits of span that Singe needed. If you have a more complete / more compliant version of span, that would probably be better than this. I wasn't putting this into Ports-of-Call because it's a high-quality, GPU-compatible implementation of span, but because it's something Singe has that other XCAP libraries may find useful. And if something better than this already exists then I have zero concerns about throwing this out and using the better version so long as it covers all the necessary use-cases.
  • mdspan: In principle I like the idea of aiming for something that will eventually be replaced by a standard container, but I've not looked at PortableMDArray so I can't comment on any differences between them.

@Yurlungur
Copy link
Collaborator

In general, I'm okay with this as a placeholder awaiting the move to C++20. If it's of interest, I have some code with a more "compliant" span (the code is unfinished, but span is finished).

I would be supportive of a portable backport of span into ports-of-call if this is useful for singe. The point of @BrendanKKrueger 's contribution was as he says, just that it's minimal---only what we needed.

There is an general question with the introduction of span; the next obvious step is mdspan, but this would basically supersede PortableMDArray (and, in my opinion, this should happen). Is this the goal?

This is not the current goal. I'm open to this path, but for backwards compatibility reasons would prefer PortableMDArray wrap MDSpan if we go that path.

I think the only things that currently use PortableMDArray are spiner and tests` so they could be migrated. But I'd prefer to do it piece by piece.

All that said: our priority is not cleaning up the old code in ports-of-call. It's not perfect but it's good enough. Our priority is making sure we are supporting the needs of the libraries that depend on us in a performant way that does not compromise code maintainability.

In the long term, I'm supportive of replacing PortableMDarray with a performance portable mdspan backport. If providing performance portablemdspan will save code duplication accross the XCAP libraries, then that's something we can prioritize. But if not then it needs to be on the backburner vs more urgent priorities.

Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

LGTM at this point. @mauneyc-LANL please re-review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants