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

Derived View type and allocation #566

Closed
hcedwar opened this issue Dec 7, 2016 · 10 comments
Closed

Derived View type and allocation #566

hcedwar opened this issue Dec 7, 2016 · 10 comments
Assignees
Labels
Feature Request Create new capability; will potentially require voting
Milestone

Comments

@hcedwar
Copy link
Contributor

hcedwar commented Dec 7, 2016

View value_type deduction:

  • Given a set of View::value_type that will be used in an arithmetic expression what should be the resulting value_type
  • Analogous to ‘decltype( ( value_type...) )’
  • The dimensions and other properties are not deduced, only the value_type
  • An extensible deducer meta-function on the value_type... is needed
  • An concern is the may involve more than the standard operators (+,-,*,/) and thus a simple deducer may not be accurate in corner cases

View allocation with the deduced value_type

  • Allocation of this View type (may) requires information from the list of Views involved in the expression
  • Thus an allocation-information deducer on the View... is needed
  • This allocation-information must be passed along to for View allocation

The current Sacado view factory provides both deducers and wrapper to View allocation
https://github.com/trilinos/Trilinos/blob/master/packages/sacado/example/view_factory_example.cpp

We need an extensible way to provide this functionality in order to remove the dependency of Intrepid2 on Sacado

@hcedwar hcedwar added the Feature Request Create new capability; will potentially require voting label Dec 7, 2016
@hcedwar hcedwar added this to the Backlog milestone Jan 11, 2017
@crtrott
Copy link
Member

crtrott commented Mar 22, 2017

What is the difference of this with Arithmetic Traits which exists in KokkosKernels?

@hcedwar
Copy link
Contributor Author

hcedwar commented Apr 27, 2017

@kyungjoo-kim @mperego @ndellingwood @etphipp
Would something like the following design satisfy the use case / use pattern?

ViewX x ;
ViewY y ;
ViewZ z ;
using value_type = typename decltype( deduce_view_specialization( x , y , z ) )::value_type ;
View< value_type ** , Space >  w( view_alloc( "w" , deduce_view_specialization( x , y , z ) ) , n0 , n1 );

Same discussion as issue #253

@etphipp
Copy link
Contributor

etphipp commented Apr 27, 2017

I guess it all depends on what deduce_view_specialization returns, and how complicated it is to use. What would you propose for that? Note that at some point, derived views need to be created manually (e.g., one of x, y, or z in your example), so whatever object is created by deduce_view_specialization needs to be easy to work with.

@mperego
Copy link

mperego commented Apr 28, 2017

@ndellingwood asked me to provide a code snippet from Albany to see how views are created there.
Here is an example, it runs on host, and might not be the best implementation:

`// ScalarT can be a Fad type or a double.
using DynRankViewScalarT = Kokkos::DynRankView<ScalarT, PHX::Device>;
PHX::MDField<const ScalarT,Cell,Node> dof; //it stores a DynRankViewScalarT view.
DynRankViewScalarT dofSide_buffer, dofSide;

… // dof is allocated

// create unmanaged view for buffer using information from dof
dofSide_buffer = Kokkos::createDynRankView(dof.get_view(), "dofSide", numCells*maxNumQpSide);

...

// create dofSide view using buffer. This could be on device.
dofSide = Kokkos::createViewWithType(dofSide_buffer, dofSide_buffer.data(), numCells_, numQPsSide);`

@hcedwar
Copy link
Contributor Author

hcedwar commented Apr 28, 2017

@etphipp re: deduce_view_specialization
For ordinary views a simple tag; e.g., struct ViewNotSpecialized { using value_type = ... }; .
For special views a bit less simple; e.g.,

struct ViewSacadoSpecialization {
  using value_type = // deduced with specializations meta function
  unsigned embedded_dimension ;
};

specific name of function is t.b.d.

@etphipp
Copy link
Contributor

etphipp commented May 1, 2017

I think I would have to see it in action to really know for sure. Are you thinking this would replace the existing approach completely, e.g.,

View<FadType**>("w",m,n,embedded_dimension)

or would that still be available? I would like to preserve that as an option, I guess, since it seems to me to be the most convenient way to create specialized views by hand (which is the majority of the use cases).

@ndellingwood ndellingwood self-assigned this May 17, 2017
ndellingwood added a commit to ndellingwood/kokkos that referenced this issue May 25, 2017
Add corresponding routines to Sacado to implement the desired
functionality in issue kokkos#566
ndellingwood added a commit to ndellingwood/kokkos that referenced this issue Jun 1, 2017
Parially addresses Github issue kokkos#566

Add a property to ViewCtorProp, metafunctions, and user API to store
embedded dimension within a ViewCtorProp instance, and retrieve/append
to layout when this is present during construction of the view.

TODO:
* Finalize names of newly introduced items
* Cleanup following name finalize
* Move sandbox tests into Kokkos unit test framework and Sacados
* Extensive testing

WARNING: Corresponding changes were made to Sacado's
KokkosExp_View_Fad.hpp and Kokkos_ViewFactory.hpp files and must be
applied via patch prior to next snapshot of Kokkos into Trilinos and
testing
ndellingwood added a commit to ndellingwood/kokkos that referenced this issue Jun 2, 2017
Unit test to check functionality added for issue kokkos#566

TODO: Test Cuda
@ndellingwood
Copy link
Contributor

@hcedwar I have implemented functionality for this enhancement, based on your design suggestion, available from branch of my fork
https://github.com/ndellingwood/kokkos/tree/feature-566

Let me know the best way to review, particularly name choices of functions, meta-functions, types etc. introduced. Following finalization of names and full testing I'll test issue PR following successful testing.

Corresponding Sacado changes are stored in a patch that I'll attach after naming is final. The patch will need to be applied to Trilinos and committed synchronously with next Kokkos snapshot and integration testing.

@kyungjoo-kim
Copy link
Contributor

kyungjoo-kim commented Jun 8, 2017

@etphipp @ndellingwood

With a discussion with Eric P., I leave a comment for a robust way to construct workspace in the functor.

Inside of a functor, I sometimes need to use stack memory and wrap the memory with a view to use API interfaced via Kokkos view (I do not want to use shared memory as it limits parallel for with a team policy only). My use case is

struct Functor {
  // A may be double or SFAD or DFAD or what so ever...
  ViewType A;
  void operator(int i) cosnt {
     constexpr int bufSize = MaxOrder;

     // Option 1. possible mistake
     typename ViewType::value_type buf[BufSize];  
     // this may call FAD constructor and destructor and cause segmentation fault.

     // Option 2. mostly okay but still possible mistake
     char buf[BufSize*sizeof(typename ViewType::value_type)];
     // this does not call FAD constructor nor destructor so it does not get the trouble as the above have
     // the empty memory can be just used as workspace
     // according to Eric P's comment, this is not still generic as some FAD just have a pointer and 
     // raw data is somewhere else. 

     // Option 3. correct way 
     const int fad = (Kokkos::is_view_fad<ViewType>::value ? Kokkos::dimension_scalar(A) : 1);
     typename ViewType::non_const_scalar_array_type::value_type buf[bufSize*(fad+1)];

     // wrapping workspace with a view
     Kokkos::DynRankView<typename ViewType::value_type, 
                                        Kokkos::Impl::ActiveExecutionMemorySpace> 
      work((typename ViewType::pointer_type)&buf[0], bufSize);
   
     doSomethingWithWork(A, work);
  }
};

The third one is correct but it is still impl dependent and not insulated from fad specific data layout.

Can we have a more robust and systematic way to create static workspace ?

Best
Kyungjoo

@hcedwar
Copy link
Contributor Author

hcedwar commented Jun 8, 2017

namespace Impl {
template< class ... Views >
struct CommonViewSpecialize ; // Recursively determine the one common non-void ViewTraits<...>::specialize among the view types

template< class SpecializationTag , class ... Views >
struct CommonViewTraits ; // specialized for ViewTraits<...>::specialize

template < class ... Views >
struct CommonViewTraits< void , Views ... > {
  using value_type = typename std::common_type< typename Views::value_type ... >::type ;
};

// appears only in Sacado
template< class ... Views >
struct CommonViewTraits< ViewSpecializeSacadoFad , Views ... > {
  using value_type = ... ;
  unsigned fad_dimension ;
};

template< class ... Views >
using DeduceCommonViewTraits = 
  CommonViewTraits< typename CommonViewSpecialize< Views ... >::specialize , Views ... > ;
}

template< class ... Views >
Impl::DeduceCommonViewTraits< Views ... >
common_view_traits( Views const & ... args )
{  return Impl::DeduceCommonViewTraits< Views ... >( args... ); }

ndellingwood added a commit to ndellingwood/kokkos that referenced this issue Jun 20, 2017
@ndellingwood
Copy link
Contributor

@hcedwar Have a functional second impl based on your design suggestion above. It helped clean up some of the metafunctions and moved the Sacado-specific implementation details to Sacado.

A couple comments:

  1. The CommonViewTraits type defined above cannot be used within a ViewMapping (in Sacado to retrieve the stored 'common' fad_dim), since the ViewCtorProp cannot be casted to this property without the Views. I added a struct that is templated on the value_type and specialize trait; it stores the same type info (and fad_dim for Sacado), but allows for retrieval via cast of the ViewCtorProp within ViewMapping (specifically in Sacado's allocate_shared routine prior to creation and assignment to m_offset, where the correct layout is required by this point).

  2. This functionality (see April 27 comment) is supposed to work with Views of pod or specialize types (i.e. FadType); in addition, my understanding is it should work whether Sacado is enabled or disabled. I had to add a specialization to the ViewCtorProp in Kokkos to handle this - the CommonViewTraits struct is essentially a tag with type info in the Kokkos case. I realize it is not desirable to modify Kokkos core, but no alternative was clear to me. We can discuss following review of the code and if a more desirable alternative is identified I'll modify accordingly.

Kokkos impl is on this branch of my fork: ndellingwood@5d2fde7

Sacado impl is stored locally and in a patch, will attach or email prior to review.

@hcedwar hcedwar modified the milestones: 2017-June-end, Backlog Jun 29, 2017
ndellingwood added a commit to ndellingwood/kokkos that referenced this issue Jun 30, 2017
ndellingwood added a commit to ndellingwood/kokkos that referenced this issue Jun 30, 2017
Addresses issue kokkos#566 to provide functionality in Kokkos to
replace ViewFactory in Sacado
ndellingwood added a commit to ndellingwood/kokkos that referenced this issue Jun 30, 2017
Addresses issue kokkos#566 to provide functionality in Kokkos to
replace ViewFactory in Sacado
hcedwar added a commit that referenced this issue Jul 5, 2017
Issue #566: Add metafunctions and prop to deduce common value_type
@crtrott crtrott closed this as completed Jul 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Create new capability; will potentially require voting
Projects
None yet
Development

No branches or pull requests

6 participants