-
Notifications
You must be signed in to change notification settings - Fork 229
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
Pull out conv2d kernels from op #320
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.
Overall, it looks good to me. I have one question that's related to the codebase. The .cpp
file seem to mimic its corresponding .cu
file. Is it auto-generated? If not, should that also be modified for this PR?
Also, what is the purpose of the kernel wrappers? If there's a forward
and a backward
and then also have forward_kernel
and backward_kernel
, I'm wondering if the wrapper is really necessary because it mostly seems to be doing some profiling.
Blocked on #345 as currently I can't check that the AMD kernel changes are correct |
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.
What is the purpose of the change from typename
to class
in model.h
? Otherwise LGTM
This is a C++17 feature apparently 🤷, whereas |
By pulling out the kernel functions into their own files, we should decrease build time by reducing the number of times the kernels (which rarely change) need to be recompiled