Addresses some Tribol requests. #164
Conversation
corbett5
force-pushed
the
feature/corbett/no-chai
branch
3 times, most recently
from
a6c5fe0 to
dc335f2
Compare
corbett5
force-pushed
the
feature/corbett/no-chai
branch
from
c323acf to
4d22d70
Compare
| @@ -220,7 +220,7 @@ class ArrayOfR2TensorsRAJA : private ArrayOfR2TensorsNative< PERMUTATION > | |||
| } | |||
|
|
|||
| ~ArrayOfR2TensorsRAJA() | |||
| { this->m_c.move( chai::CPU ); } | |||
| { this->m_c.move( MemorySpace::CPU ); } | |||
There was a problem hiding this comment.
Is this a move from a chai scope to an umpire scope?
There was a problem hiding this comment.
It's an enum I defined myself. I think camp and umpire both have similar features but this works fine for now.
| endif() | ||
|
|
||
| if(NOT EXISTS ${CHAI_DIR}) | ||
| set(CHAI_DIR ${GEOSX_TPL_DIR}/chai) |
There was a problem hiding this comment.
are we still dependent on the your patch to chai?
There was a problem hiding this comment.
The patch in the tpl repo needs to be applied until the next release. But building off of the main release and not my feature/corbett/changes-for-geosx branch.
| */ | ||
| template< typename ... DIMS > | ||
| LVARRAY_HOST_DEVICE | ||
| void resize( DIMS... newDims ) | ||
| std::enable_if_t< sizeof ... ( DIMS ) == NDIM && all_of_t< std::is_integral< DIMS > ... >::value > |
There was a problem hiding this comment.
what is the return type here?
There was a problem hiding this comment.
void
template< bool CONDITION, typename RETURN=void >
using enable_if_t = typename std::enable_if< CONDITION, RETURN >;| push_back( T const & value ) | ||
| template< typename ... ARGS, int _NDIM=NDIM > | ||
| std::enable_if_t< _NDIM == 1 > | ||
| emplace_back( ARGS && ... args ) |
There was a problem hiding this comment.
i don't think you want to get rid of push_back?
There was a problem hiding this comment.
emplace_back is a drop in replacement for push_back and it's supported by std::vector as well. I would prefer not to also have push_back because it would add two methods push_back( T const &) and push_back( T && ) that add no new functionality.
| { return reinterpret_cast< CRSMatrixView< T, COL_TYPE, INDEX_TYPE const > const & >( *this ); } | ||
| CRSMatrixView< T, COL_TYPE, INDEX_TYPE const, BUFFER_TYPE > const & | ||
| toView() const LVARRAY_RESTRICT_THIS | ||
| { return reinterpret_cast< CRSMatrixView< T, COL_TYPE, INDEX_TYPE const, BUFFER_TYPE > const & >( *this ); } |
There was a problem hiding this comment.
We do need to get rid of the reinterpret_cast at some point. This would be replaced by the construction of a view, and the return of that view rather than a reference. Can this be done without downstream changes?
There was a problem hiding this comment.
Yes everything should compile since we currently have
CRSMatrixView< T > const & CRSMatrix< T >::toView() const
{ return reinterpret_cast<CRSMatrixView< T > const & >( *this ); }
...
CRSMatrix< T > matrix;
CRSMatrixView< T > const & view = matrix.toView();and if we change `toView() to return by value it will still work
CRSMatrixView< T > CRSMatrix< T >::toView() const
{ return CRSMatrixView< T >( m_entries, m_values, m_sizes, m_offsets ); }
...
CRSMatrix< T > matrix;
CRSMatrixView< T > const & view = matrix.toView();However I think best practices dictate that now that toView returns by value we they type of view should be CRSMatrixView< T > const and not CRSMatrixView< T > const &.
Furthermore there could be runtime issues if the user writes code that assumes the view and the owner are the same object. Currently this would pass
Array1d< int > array;
ArrayView1d< int > const & view = array.toView();
array.resize( 1 );
EXPECT_EQ( view.size(), 1 );
EXPECT_NE( view.data(), nullptr );However once we switch to returning by value this will fail.
| return chai::CPU; | ||
| #if defined(USE_CUDA) | ||
| if( space == MemorySpace::GPU ) | ||
| return chai::GPU; |
There was a problem hiding this comment.
Do we even want to use the chai execution spaces? It seems like they should just alias the MemorySpace
There was a problem hiding this comment.
The Chai api needs a chai::ExecutionSpace so we need to have a method to convert between the two.
| * @enum MemorySpace | ||
| * @brief An enum containing the available memory spaces. | ||
| */ | ||
| enum class MemorySpace |
There was a problem hiding this comment.
Does umpire have a similar enum?
There was a problem hiding this comment.
Probably, I think camp does too.
corbett5
force-pushed
the
feature/corbett/no-chai
branch
from
3112967 to
55b8c4f
Compare
USE_OPENMPandUSE_CUDA.RAJAandCHAIto build against.NDIMtemplate parameter ofArray.push_backwithemplace_back.insertwithemplace.beginandendtoArraySliceand removedArrayOfArrayView::getIterableArray.TYPED_TESTand there are no longer any tests that are only run with CUDA.