C++20: bump camp submodule and modify SFINAE to ensure compatibility #2020
C++20: bump camp submodule and modify SFINAE to ensure compatibility #2020johnbowen42 wants to merge 14 commits into
Conversation
… with new concepts
johnbowen42
changed the title
| template<typename IdxT = hip_dim_member_t, | ||
| typename IdxNDims = NonCachedIndicesAndDims> | ||
| RAJA_DEVICE static constexpr IdxT size(IdxNDims const& idxNDims = IdxNDims {}) | ||
| RAJA_DEVICE static constexpr IdxT size(IdxNDims const& = IdxNDims {}) |
There was a problem hiding this comment.
This is actually an unrelated change--but I was getting unused variable warnings from this file while working on the HIP backend. I can remove and put in a separate PR if reviewers would like.
There was a problem hiding this comment.
If this is in hip, then it should also be in cuda.
| // TODO(bowen): Did we ever support passing these types to Span? They do not | ||
| // model Integral |
There was a problem hiding this comment.
Good question, did the tests using StrongIdxTypeList work before, but don't now?
There was a problem hiding this comment.
yes they did--I am trying to figure out why now. Somehow we were instantiating Span with StrongIndexType before
There was a problem hiding this comment.
There was a bug with the old concept approach--if you replace
static_assert(type_traits::is_integral<IndexType>::value, "IndexType must model Integral"); with std::is_integral_v inside the Span class template definition in main the same tests fail to compile
There was a problem hiding this comment.
Ok, so I changed the kernel tests (kernel uses span to create segments) to use a different list defined by this header. Let me know what you think
There was a problem hiding this comment.
I guess what we really want is something to convertible to integral?
There was a problem hiding this comment.
I appreciate that we want to support that, but StrongTypeIndex did not look like an int (see
). Even the index types that are meant to look like ints here are commented out of the test suite. I think our "support" for that index type through our test suite was largely an accident of the implementation of camp's conceptsThere was a problem hiding this comment.
I think we should open an issue and revisit this aspect of the testing suite separate from this PR, because it looks like we were trying to support these types of indices but not actually supporting them in reality
There was a problem hiding this comment.
I'm open to making the change here as well, but I think it would expand the scope of this PR
There was a problem hiding this comment.
@artv3 @MrBurmark Ok I thought about this more and I think I have a reasonable solution--see the new SpanIndex concept here https://github.com/llnl/RAJA/pull/2020/changes#diff-38e454fee010a2097ab28fc9773b94e23501f140ce26ebd7acb5da2402f0e052R317 and https://github.com/llnl/RAJA/pull/2020/changes#diff-b36ff9f682b4cc0577fac7779445c486b66e2abef955db1227a7b9cca3128ff7R68. I tried using the same trick with the other StrongInt and StrongULL types in the https://github.com/llnl/RAJA/blob/4ea83c49c874476fc0bec9500902ef527d240a95/test/include/RAJA_test-index-types.hpp, but compilation failed here, so that is still an open task
It's a bit ambiguous whether Span was intended to be used this way. The tests seem to make it seem so, but the old static asserts seem to indicate otherwise. In any case, this new concept fixes compilation and restores test coverage.
There was a problem hiding this comment.
If we think of Span as being similar to View then yes, but if its just something that gives you begin and end iterators then maybe not.
| * | ||
| */ | ||
| template<typename IterType, typename IndexType> | ||
| template<concepts::RandomAccessIterator IterType, concepts::Integral IndexType> |
There was a problem hiding this comment.
Will this still work with strong index types?
There was a problem hiding this comment.
Nope, and it never should have based on the static assert before. See here #2020 (comment)
|
FYI: I did a short review. Once the first batch of comments are addressed, I am happy to revisit the code. |
| type_traits::is_resource<Res>, | ||
| std::is_constructible<camp::resources::Resource, Res>, | ||
| type_traits::is_range<Container>> | ||
| std::is_constructible<camp::resources::Resource, Res>> |
There was a problem hiding this comment.
Is this not required for anything that satisfies concepts::Resource?
| concepts::Range InContainer, | ||
| concepts::Range OutContainer, |
There was a problem hiding this comment.
Shouldn't these be RandomAccessRanges, given that this is just a wrapper around a function that requires that?
| namespace concepts | ||
| { | ||
| template<typename T> | ||
| concept SpanIndex = concepts::Integral<T> || concepts::IndexValued<T>; |
There was a problem hiding this comment.
I imagine we're going to need this concept more generally in the future, so it should probably be in a more general location and have a more general name.
There was a problem hiding this comment.
In general, I have been wondering if our approach to creating concepts across many headers is the right approach. Should we try to consolidate all our definitions into a single header? I feel like this might be useful for understanding RAJA for new developers
There was a problem hiding this comment.
I think that would be good for most of the general concepts. I wonder how many things we'll have to move around so we actually can do this.
| Tester<Index> test1; // should compile | ||
| // Tester<Ty> test2; // should not compile |
6 participants
This PR does more than just bump the camp submodule. Changing the submodule required some modernization and changes to ensure compatibility.