Adding data copy between host and device#323
Adding data copy between host and device#323hochawa wants to merge 8 commits intotensor-compiler:masterfrom
Conversation
stephenchouca
left a comment
stephenchouca
left a comment
There was a problem hiding this comment.
Looks pretty good overall! I think it'd be good to add one or two test cases with input tensor(s) that are stored in a zeroless format though to make sure everything works.
src/lower/mode_format_singleton.cpp
Outdated
| } | ||
|
|
||
| SingletonModeFormat::SingletonModeFormat(bool isFull, bool isOrdered, | ||
| bool isUnique, long long allocSize) : |
There was a problem hiding this comment.
Shouldn't this constructor also be modified to take in isZeroless as an argument?
There was a problem hiding this comment.
Oh, you are right. Thank you. By the way, it is wired because I modified the code as you suggested about a week ago, but the pull request does not seem to reflect it.
src/lower/iterator.cpp
Outdated
|
|
||
| bool Iterator::isZeroless() const { | ||
| taco_iassert(defined()); | ||
| if (isDimensionIterator()) return true; |
There was a problem hiding this comment.
Should return false for dimension iterators I think?
There was a problem hiding this comment.
Oh, yes. it should return false for dimension iterators. Thank you for correcting me.
|
Could you accept the feature for cudaMalloc and cudaMemcpy, or give me a comment? |
hochawa
changed the title
src/codegen/codegen.h
Outdated
|
|
||
| #define PRINT_FUNC 0 | ||
| #define PRINT_MEM_HOST_TO_DEV 1 | ||
| #define PRINT_MEM_DEV_TO_HOST 2 |
There was a problem hiding this comment.
Should replace these with an enum.
There was a problem hiding this comment.
Oh, I see. I will replace them with an enum.
| taco_iassert(scopeMap.count(var) == 0) << | ||
| "Duplicate output found in codegen"; | ||
|
|
||
| output_tensor = var->name; // Isn't there only one output? |
There was a problem hiding this comment.
The code generator was designed with the thought that we might eventually support kernels with multiple outputs, though that's not something we've actually fully implemented. It's probably fine to assume for now that there's only one output, though you should probably add an assertion to check for that (e.g., taco_iassert(outputs.size() == 1)) .
There was a problem hiding this comment.
I see. Thanks for explanation. I know understand why the code looks like that. I will add an assertion to check there is only one output.
There was a problem hiding this comment.
Thanks for your time to take a look at the parts I modified!
|
I've added a couple more comments above. On the whole the changes look good though; I'll merge them into master after you've addressed those comments. |
2 participants
Could you give me comments or accept the pull request?