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.

Sign up for GitHub

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

Jump to bottom

Addresses some Tribol requests. #164

Merged
merged 1 commit into from
Jun 19, 2020
Merged

Addresses some Tribol requests. #164

merged 1 commit into from
Jun 19, 2020

Conversation

corbett5
Copy link
Collaborator

  • Now uses vanilla Chai 2.1.1, and can build without.
  • Compiles with all four combinations of USE_OPENMP and USE_CUDA.
  • Can specify the path to RAJA and CHAI to build against.
  • Every class is now templated on the buffer type.
  • Removed all default template arguments.
  • Removed the NDIM template parameter of Array.
  • Replaced push_back with emplace_back.
  • Replaced single valued insert with emplace.
  • All multi valued append/insertion methods take iterators instead of a pointer and a size.
  • Added begin and end to ArraySlice and removed ArrayOfArrayView::getIterableArray.
  • All of the unit tests use TYPED_TEST and there are no longer any tests that are only run with CUDA.

corbett5
corbett5 commented Jun 17, 2020
View reviewed changes
src/Array.hpp Outdated Show resolved Hide resolved
@corbett5 corbett5 requested a review from rrsettgast June 17, 2020 02:25
@corbett5 corbett5 force-pushed the feature/corbett/no-chai branch 3 times, most recently from a6c5fe0 to dc335f2 Compare June 17, 2020 06:41
@corbett5 corbett5 mentioned this pull request Jun 17, 2020
Merged
rrsettgast
rrsettgast approved these changes Jun 18, 2020
View reviewed changes
benchmarks/benchmarkArray1DR2TensorMultiplicationKernels.cpp Show resolved Hide resolved
benchmarks/benchmarkArray1DR2TensorMultiplicationKernels.hpp
@@ -220,7 +220,7 @@ class ArrayOfR2TensorsRAJA : private ArrayOfR2TensorsNative< PERMUTATION >
}

~ArrayOfR2TensorsRAJA()
{ this->m_c.move( chai::CPU ); }
{ this->m_c.move( MemorySpace::CPU ); }
Copy link
Member

Choose a reason for hiding this comment

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

Is this a move from a chai scope to an umpire scope?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's an enum I defined myself. I think camp and umpire both have similar features but this works fine for now.

cmake/SetupTPL.cmake
endif()

if(NOT EXISTS ${CHAI_DIR})
set(CHAI_DIR ${GEOSX_TPL_DIR}/chai)
Copy link
Member

Choose a reason for hiding this comment

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

are we still dependent on the your patch to chai?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

src/Array.hpp
*/
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 >
Copy link
Member

Choose a reason for hiding this comment

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

what is the return type here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

void

template< bool CONDITION, typename RETURN=void >
using enable_if_t = typename std::enable_if< CONDITION, RETURN >;

src/Array.hpp
push_back( T const & value )
template< typename ... ARGS, int _NDIM=NDIM >
std::enable_if_t< _NDIM == 1 >
emplace_back( ARGS && ... args )
Copy link
Member

Choose a reason for hiding this comment

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

i don't think you want to get rid of push_back?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

src/CRSMatrix.hpp
{ 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 ); }
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

src/NewChaiBuffer.hpp
return chai::CPU;
#if defined(USE_CUDA)
if( space == MemorySpace::GPU )
return chai::GPU;
Copy link
Member

Choose a reason for hiding this comment

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

Do we even want to use the chai execution spaces? It seems like they should just alias the MemorySpace

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The Chai api needs a chai::ExecutionSpace so we need to have a method to convert between the two.

src/bufferManipulation.hpp
* @enum MemorySpace
* @brief An enum containing the available memory spaces.
*/
enum class MemorySpace
Copy link
Member

Choose a reason for hiding this comment

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

Does umpire have a similar enum?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably, I think camp does too.

src/streamIO.hpp Show resolved Hide resolved
src/templateHelpers.hpp Show resolved Hide resolved
@corbett5 corbett5 mentioned this pull request Jun 18, 2020
Open
@corbett5 corbett5 merged commit 090da89 into develop Jun 19, 2020
@corbett5 corbett5 deleted the feature/corbett/no-chai branch June 19, 2020 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rrsettgast rrsettgast rrsettgast approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@corbett5 @rrsettgast