nv-legate / cupynumeric

An Aspiring Drop-In Replacement for NumPy at Scale
https://docs.nvidia.com/cupynumeric
Apache License 2.0
623 stars 71 forks source link

src/cunumeric/matrix/transpose.cu skips transpose_2d_logical function #1107

Closed XiaLuNV closed 9 months ago

XiaLuNV commented 11 months ago

I'am working on the C++ coverage for transpose, found that transpose_2d_logicalwas skipped.

Codes in cunumeric/cunumeric/array.py ,in the initialization method,

self._thunk = runtime.find_or_create_array_thunk(
                    np_array, share=False
                )

Codes in cunumeric/cunumeric/runtime.py, find_or_create_array_thunk has these codes:

            # This is not a scalar so make a field
            store = self.legate_context.create_store(
                dtype,
                shape=array.shape,
                optimize_scalar=False,
            )

the optimize_scalar is set to False, did this mean the Array is stored in physical way and the logicalvalue in struct TransposeImplBody(src/cunumeric/matrix/transpose.cu) is False, so it only calls transpose_2d_physicaland skips transpose_2d_logical.

Codes in src/cunumeric/matrix/transpose.cu:

    if (logical)
      transpose_2d_logical<VAL>
        <<<blocks, threads, 0, stream>>>(out, in, in_rect.lo, in_rect.hi, out_rect.lo, out_rect.hi);
    else
      transpose_2d_physical<VAL>
        <<<blocks, threads, 0, stream>>>(out, in, in_rect.lo, in_rect.hi, out_rect.lo, out_rect.hi);
manopapad commented 10 months ago

This doesn't have to do with the initialization of an array. It's just that the "logical" case in the TRANSPOSE_COPY_2D task is unreachable code currently, because both places where it's invoked pass logical=False:

@magnatelee should we get rid of the "logical" case? Otherwise maybe we add a way to test it.

magnatelee commented 10 months ago

I think we should. I remember we had a use case for the physical code path at some point, but I suspect that got replaced with the code using a transpose transformation.

manopapad commented 10 months ago

Should be fixed by #1113