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

dash::Matrix doesn't accept struct as template datatype #241

Closed
BenjaProg opened this issue Jan 19, 2017 · 13 comments · Fixed by #293
Closed

dash::Matrix doesn't accept struct as template datatype #241

BenjaProg opened this issue Jan 19, 2017 · 13 comments · Fixed by #293
Assignees
Labels

Comments

@BenjaProg
Copy link
Member

BenjaProg commented Jan 19, 2017

Why does dash::Matrix and thus dash::NArray not accept struct as template datatype while dash::Array does?
For example:

using Point = struct{ MTRX_TYPE value;
                      uint row, col;};
dash::Array  < Point   > points( nelts );  // ok
dash::NArray < Point,1 > points( nelts );  // bad
dash::Matrix < Point,1 > points( nelts );  // bad
@devreal
Copy link
Member

devreal commented Jan 19, 2017

My guess would be that dash::Array is just missing the static_assert, hence the different behavior. However, afaics both could potentially store non-trivial (but trivially-copyable) data but I am not sure on the design decisions taken earlier.

@dhinf
Copy link
Member

dhinf commented Jan 19, 2017

Matrix accepts structs, but all members have to be trivially-copyable. How is MTRX_TYPE defined?

@devreal
Copy link
Member

devreal commented Jan 19, 2017

Interestingly we enforce is_trivial for Matrix but not for Array, which has the following comment in it:

Check requirements on element type
is_trivially_copyable is not implemented presently, and is_trivial
is too strict (e.g. fails on std::pair).

For details on the two: http://stackoverflow.com/questions/4178175/what-are-aggregates-and-pods-and-how-why-are-they-special/7189821#7189821

is_trivially_copyable seems to be implemented with GCC >5.1.0 but not with (the rather ancient) <=4.9.4: https://gcc.gnu.org/onlinedocs/gcc-5.1.0/libstdc++/manual/manual/status.html#status.iso.2011

In any case, the behavior should be consistent.

@fuerlinger
Copy link
Contributor

95.datatypes.pdf

Here is an old "design document" on this issue. is_trivially_copyable was indeed not implemented back then. If it is available now on all platforms of interest to us then it seems like this is the appropriate property to check for. Structs are certainly useful to have as elemental types in our data structures.

@BenjaProg
Copy link
Member Author

@dhinf MTRX_TYPE is defined: using MTRX_TYPE = uchar;
In the program a matrix is read in and depending onthe values the programmer can/should change the type of the matrix. The program is templated in a way so you can just change MTRX_TYPE.
And in the use cases of this program, the type of MTRX_TYPE should always be trivially copyable.

@dhinf
Copy link
Member

dhinf commented Jan 20, 2017

How is uchar defined? I tried a struct with unsigned char and it worked.

@devreal
Copy link
Member

devreal commented Jan 20, 2017

@fuerlinger
I did some tests with different compilers that I have at hand: Intel seems to support it since ICC 15, GNU since 5.1.0, and Clang 3.8.0 supports it as well (although support might be available in earlier versions of Clang but this is the earliest version I have access to atm).

The only exception is the Cray compiler: it uses the g++ header files of version 4.8.2 (!), even in the latest CCE 8.5.6. Loading a different GNU version does not change this situation. The Cray compiler claims to support C++11, so this is clearly a missing feature. Will file a bug report :)

@BenjaProg
Copy link
Member Author

@dhinf Yes uchar is just a unsigned char.
The code i am talking about is in this line.

remark: the code is a construction site and incomplete, but runs so far...
The dash::Array in Line 80 is not needed at the moment because of design decisions.

@fuchsto
Copy link
Member

fuchsto commented Jan 20, 2017

@devreal Yeah, Cray, much like OpenMPI, are speshul and spoil the fun for everyone. Please don't invest your time in fixing their issues. As far as I'm concerned, I'm fine with something like #ifdef DASH_ARCH_CRAY.

@devreal
Copy link
Member

devreal commented Jan 23, 2017

The Cray on-site support escalated this issue to the compiler group so it might get fixed at some point. In the mean-time, we will guard the check through the _CRAYC macro.

I adapted the containers in branch bug-241-containertypes but it currently fails to compiler ex.04.memalloc, where a GlobPtr is used within a dash::Array. @fuerlinger can you please check this example? Apart from not being trivially_copyable (non-trivial assignment operator), it's also not safe to copy across units in general, see my comments in #221.

@devreal
Copy link
Member

devreal commented Jan 23, 2017

Some more observations:

  • STLAlgorithmTest fails to compile because std::pair is not trivially copyable.
  • bench.04.histo fails to compile because of a dash::Array of GlobPtr.
  • app.01.mindeg fails to compile because node_t has a member of type GlobPtr.

@fuerlinger
Copy link
Contributor

Thanks for escalating the issue with Cray!

On having a GlobPtr as a elemental type in a DASH container I think we definitely should try to allow that if possible at all. This is a fundamental way to build more complex data structures out of the basics that are offered out of the box.

And that of course ties in with #221 : I think we should really allow to copy global pointers between units, again if we don't allow that then the value of a global pointer is greatly diminished and it will make many things more complex and costly down the road. And I think we can make it happen with not too much effort. The issue is that segment IDs are not consistent across units, correct? I think we can find a way to fix that.

@devreal
Copy link
Member

devreal commented Jan 23, 2017

I fully agree that a GlobPtr (and the underlying gptr) should be sharable between units. This has been haunting me before as well. After #221 has been resolved, the trivial fix is to make the copy assignment operator default.

Regarding std::pair: As mentioned by @fuchsto, having a pair type in containers is certainly handy. We could easily provide our own implementation that is trivially_copyable.

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

Successfully merging a pull request may close this issue.

7 participants