make permutation path in cutensor optional, fix cmake, fix setting us…#39
Conversation
…e_device_memory and demo-dynamic, test-dynamic
|
Regarding After adding $ ./tapp-reference-test-dynamic
Starting seed for random numbers = 0
Hadamard Product: true
Contraction: true
Commutativity: true
Permutations: true
Equal Extents: true
Outer Product: true
Full Contraction: true
Zero Dim Tensor Contraction: true
One Dim Tensor Contraction: true
Subtensor Same Index: true
tapp-reference-test-dynamic: /home/urza/Projects/scratch/tapp-torch/third_party/tapp/cutensor_bindings/src/product.cu:220:
TAPP_error TAPP_execute_product(TAPP_tensor_product, TAPP_executor, TAPP_status*, const void*, const void*, const void*, const void*, const void*, void*):
Assertion `uintptr_t(A_d) % 128 == 0' failed. |
jurajHasik
force-pushed
the
feat_skip_permute
branch
from
7069097 to
389423c
Compare
|
|
||
| TAPP_EXPORT TAPP_error TAPP_attr_set(TAPP_attr attr, TAPP_key key, void* value); | ||
|
|
||
| TAPP_EXPORT TAPP_error TAPP_attr_get(TAPP_attr attr, TAPP_key key, void** value); |
There was a problem hiding this comment.
I believe this should still be a void** because we want to return by value a void*
| return 0; | ||
| } | ||
|
|
||
| TAPP_error TAPP_attr_get(TAPP_attr attr, TAPP_key key, void** value) |
There was a problem hiding this comment.
Same as for the header file
cutensor_bindings/src/product.cu
Outdated
| @@ -1,4 +1,5 @@ | |||
| #include "../include/product.h" | |||
| #include "../include/attributes.h" | |||
There was a problem hiding this comment.
Rather put it in the header file in my opinion
cutensor_bindings/src/product.cu
Outdated
| struct handle* handle_struct = (struct handle*) ((struct product_plan*) plan)->handle; | ||
| bool use_device_memory = *(bool*)((handle_struct->attributes)[0]); | ||
| bool use_device_memory; | ||
| TAPP_attr_get((TAPP_handle)handle_struct, ATTR_KEY_USE_DEVICE_MEMORY, (void*)&use_device_memory); |
There was a problem hiding this comment.
We don't want to call functions from the TAPP interface internally. It works for now but will cause problems for multi-TAPP. I do call functions from the TAPP interface internally in the reference implementation but I am fixing it in other branches.
| { | ||
| void* handle; | ||
| TAPP_error (*TAPP_attr_set)(TAPP_attr attr, TAPP_key key, void* value); | ||
| TAPP_error (*TAPP_attr_get)(TAPP_attr attr, TAPP_key key, void** value); |
There was a problem hiding this comment.
Same as the previous comments about this
SpaceyLake
left a comment
SpaceyLake
left a comment
There was a problem hiding this comment.
Aside from the comments I made, I think it looks good! If you'd like, I could fix them myself
|
@SpaceyLake thank you ! Don't worry, I should be able to update the PR according to your comments fast. |
|
Btw: $ gdb ./tapp-reference-test-dynamic
[...]
Reading symbols from ./tapp-reference-test-dynamic...
(gdb) r
Starting program: [...]/tapp-torch/third_party/tapp/build/tapp-reference-test-dynamic
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Starting seed for random numbers = 0
[New Thread 0x7fffb9fff640 (LWP 33085)]
[New Thread 0x7fffb93ff640 (LWP 33088)]
[Thread 0x7fffb93ff640 (LWP 33088) exited]
[New Thread 0x7fffb93ff640 (LWP 33089)]
[New Thread 0x7fffaca20640 (LWP 33091)]
Hadamard Product: true
Contraction: true
Commutativity: true
Permutations: true
Equal Extents: true
Outer Product: true
Full Contraction: true
Zero Dim Tensor Contraction: true
One Dim Tensor Contraction: true
Subtensor Same Index: true
tapp-reference-test-dynamic: [...]/tapp-torch/third_party/tapp/cutensor_bindings/src/product.cu:222: TAPP_error TAPP_execute_product(TAPP_tensor_product, TAPP_executor, TAPP_status*, const void*, const void*, const void*, const void*, const void*, void*): Assertion `uintptr_t(A_d) % 128 == 0' failed.
Thread 1 "tapp-reference-" received signal SIGABRT, Aborted.
__pthread_kill_implementation (no_tid=0, signo=6, threadid=140737350686528) at ./nptl/pthread_kill.c:44
44 ./nptl/pthread_kill.c: No such file or directory. |
SpaceyLake
left a comment
SpaceyLake
left a comment
There was a problem hiding this comment.
I am happy with the changes. The problems with test-dynamic seem to exist on the original branch as well. I'll merge this and fix it on the branch
2 participants
fix CMake build: expose
CUTENSOR_INCLUDE_DIRto the rest of CMake (i.e. cutensor dependent targets in top-level CMakeLists)fix
TAPP_attr_get/setand its use in demo-dynamic, test-dynamic to setuse_device_memory.This fixes demo-dynamic. The test-dynamic fails for other reasons [see comment below]
Makes permutation path in cutensor_bindings
TAPP_execute_productoptional: Ifop_D == TAPP_IDENTITY, permutation path is skipped, which saves on unnecessary allocation/deallocation of the intermediatememory in
TAPP_execute_productis managed via Async de/allocation wrt. to supplied stream (TAPP_executor). This avoids the need for stream synchronization