-
Notifications
You must be signed in to change notification settings - Fork 215
refactor(gpu): moving unsigned_scalar_div_rem and signed_scalar_div_rem to the backend #2499
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! Thank you for this PR! Here are my comments 🙂
backends/tfhe-cuda-backend/cuda/include/integer/integer_utilities.h
Outdated
Show resolved
Hide resolved
eccbfc2
to
20e3d03
Compare
20e3d03
to
263179e
Compare
263179e
to
13da67d
Compare
13da67d
to
42bd244
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @enzodimaria .
I added some comments regarding format and consistency within the backend. I'm not capable of reviewing the logic, though.
backends/tfhe-cuda-backend/cuda/include/integer/integer_utilities.h
Outdated
Show resolved
Hide resolved
this->params = params; | ||
this->allocate_gpu_memory = allocate_gpu_memory; | ||
|
||
this->bitop_mem = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can move this type of initialization to the variable definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this->bitop_mem
is only useful if is_divisor_power_of_two
is true, I think it's better to keep this default initialization to nullptr
, in order not to allocate potentially unused objects. That was a recommendation from Agnès. What do you think?
this->bitop_mem = nullptr;
...
if (is_divisor_power_of_two) {
this->bitop_mem = new int_bitop_buffer<Torus>(
streams, gpu_indexes, gpu_count, SCALAR_BITAND, params,
num_radix_blocks, allocate_gpu_memory, size_tracker);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we need initialization but I was thinking on initializing them on declaretion, as in:
...
int_unsigned_scalar_div_mem<Torus> *unsigned_div_mem = nullptr;
int_bitop_buffer<Torus> *bitop_mem = nullptr;
int_scalar_mul_buffer<Torus> *scalar_mul_mem = nullptr;
int_sub_and_propagate<Torus> *sub_and_propagate_mem = nullptr;
This should be equivalent to what you are doing, since it's on a constructor, but more succinct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see! I can do that. But can I do it in a next PR, the one where I remove the booleans?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, other functions already merge following this logic. So for consistency with what is already merged, I think we should keep everything like this. Then in a future PR, I will re-harmonize everything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(the PR will come right after we merge this @pdroalves)
Thank you very much! |
42bd244
to
8d3c638
Compare
8d3c638
to
7682716
Compare
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
…em to the backend
badd8fd
to
acb4b32
Compare
closes: please link all relevant issues
PR content/description
Check-list: