dsharlet / array

C++ multidimensional arrays in the spirit of the STL
Apache License 2.0
198 stars 15 forks source link

Add assert_dims_compatible #64

Closed fbleibel-g closed 2 years ago

fbleibel-g commented 2 years ago

A few more things:

fbleibel-g commented 2 years ago

We're currently failing because CUDA's printf can't be found, and there's no host device implementation for std::tuple's operator =. I think this is because our build test runs with:

-nocudalib -nocudainc

(so basically no cuda standard library support, and that means our stdio.h printf is not CUDA's?).

You can ignore the latest commits - I thought this might be because travis used an older version of clang (by default, clang 7.0.0 from xenial - old!), but that didn't fix it.

I'm not sure what is the best way to address this - could we install the cuda SDK on travis? Otherwise, we can disable the new functionality if CUDA is detected?

dsharlet commented 2 years ago

We could just run the CUDA build test with asserts turned off... that's pretty lame but it seems like it would (or at least could) work around this issue, and the only thing that differs in behavior is printf, so it seems safe enough not to test it.

@jiawen any opinions? I'm not sure if it is possible to install the CUDA SDK on travis. -nocudalib is probably not necessary, because our test doesn't try to link anything, but I doubt that will fix anything anyways.

fbleibel-g commented 2 years ago

I found examples on how to install the cuda toolkit in Travis, but it's a large amount of code and the references look somewhat brittle: https://github.com/tmcdonell/travis-scripts/blob/cad952b7d62aaf006b5f83a0d7a3127e758dc9cd/install-cuda-trusty.sh

Anyway, I got it to build; I realized that Jiawen already redefined a bunch of __device__ functions in build_test.cu, so I added a variadic printf there (can't use ... in cuda, so I used parameter packs instead).

There was another error related to operator= on tuples:

In file included from test/build_test.cu:20:
./array.h:1052:11: error: reference to __host__ function 'operator=' in __host__
      __device__ function
    dims_ = other;
          ^

Jiawen also noticed this, see the note that nda::shape::operator= is also broken in build_test.cu. clang only defines constexpr functions as __host__ + __device__: https://reviews.llvm.org/D18380, and operator= is only constexpr on tuples from C++20 onwards. To bypass this, I made assert_dims_compatible return the source dims, so I can use tuple's constructor instead, and that does the trick.

This change is again ready for review.

dsharlet commented 2 years ago

This looks good now, thanks for your persistence on the CUDA issues!

fbleibel-g commented 2 years ago

Thanks for carefully reviewing my bad template code! Imported back into third_party.