Bug fixes for GPU-aware Put and Get #3090
Conversation
| @@ -1248,7 +1251,8 @@ void BP5Deserializer::FinalizeGets(std::vector<ReadRequest> Requests) | |||
|
|
|||
| helper::NdCopy(IncomingData, inStart, inCount, true, true, | |||
| (char *)Req.Data, outStart, outCount, true, | |||
| true, ElementSize); | |||
| true, ElementSize, Dims(), Dims(), Dims(), | |||
| Dims(), false, Req.MemSpace); | |||
| } | |||
There was a problem hiding this comment.
@eisenhauer This is the only part I don't like from my fix (having to spell out the default arguments), but I honestly didn't know how to make it better without breaking other parts of the code.
There was a problem hiding this comment.
@anagainaru If you want to avoid this, add MemSpace in the argument list after ElementSize rather than at the end. (ElementSize is itself a new argument, introduced a few weeks ago when NdCopy got un-templated. (The only way that templates were employed was in "sizeof(T)" and it was judged not worth having a dozen instances of the same code that differed only in that one constant, so ElementSize was made a parameter and the templating removed. But yes, the default args meant that the new parameter was best introduced before all the args with default values...)
There was a problem hiding this comment.
@anagainaru If you want to avoid this, add MemSpace in the argument list after ElementSize rather than at the end. (ElementSize is itself a new argument, introduced a few weeks ago when NdCopy got un-templated. (The only way that templates were employed was in "sizeof(T)" and it was judged not worth having a dozen instances of the same code that differed only in that one constant, so ElementSize was made a parameter and the templating removed. But yes, the default args meant that the new parameter was best introduced before all the args with default values...)
There was a problem hiding this comment.
Why not move MemSpace up to the first optional parameter? Or simply make it a compulsory parameter? Even the current looking is fine. I don't see a big problem. The ultimate goal is probably to support memory selections under all circumstances anyway, in which case the empty Dims parameters will have to be replaced with something meaningful.
There was a problem hiding this comment.
Ok I'll leave it like this for now and will figure out a better solution when we add more backends
Later edit: I think your suggestion is good, I will try it |
Bug fixes for GPU-aware Put and Get
Bug fixes include:
NdCopyfunction that broke the Get code for the BP5 engine (removed all the unused functions)Detectwhen CUDA is enabled and fixed the bugs caused by this