Conversation
|
See #1731 |
|
@slayton58 thanks for the API integration! I'll cherry-pick your commit for these changes into #1731. |
|
Let me know if there's more changes you need, or API questions that need clearing up |
|
Please lint your branch by |
|
Ok, done |
|
Hi Layton, I got cudnn-6.5-linux-x64-v2-rc2 installed ( libcudnn.so.6.5.41), with |
There was a problem hiding this comment.
Why all the reinterpret_casts?
- If we must cast,
static_castshould work here, no? - Shouldn't the cast be unneeded here, just as it's not needed for the data arguments?
There was a problem hiding this comment.
We always explicitly cast to (void *), and reinterpret_cast<void *> captures the inherent type-unsafe nature of that operation. Original commit used (void *) casting, (void *) -> reinterpret_cast<void *> was requested by your lint tool.
There was a problem hiding this comment.
Right, C-style casting is right out; whether one should use reinterpret_cast or static_cast here is a stylistic issue that can be argued either way. What bothers me is that the alpha and beta arguments are being treated differently than the data arguments, even though (according to the header I am looking at), all of those arguments have type void* and are subject to the same danger. Is there a reason for that?
There was a problem hiding this comment.
I don't see a reason that the casts have to be there, it was a stylistic decision by the author of some of our code that I followed when I wrote this patch -- I personally prefer the explicit cast, but it is extraneous
There was a problem hiding this comment.
Right, I'm not asking why the casts are there, but why the coefficients are being treated differently from the data. The code suggests that the coefficients are being passed untyped, while the data is typed, so that the data is safer than the coefficients. But from the headers it seems that everything is untyped.
Ideally one could a gain little safety with (templated, perhaps) wrappers, or perhaps y'all will introduce a typed interface at some point. But, to me it seems misleading to mark some arguments as unsafe when all are unsafe. Of course, marking every argument with reinterpret_cast would be noisy, so maybe it is better to leave them out and make a note of the unsafe interface...
|
The compile issue is because the new cuDNN drop last night changes the pooling enums slightly - this pull request was originally written for the older RC |
|
EDIT: I fixed this issue by install CUDA 7.0.
|
|
Hey @slayton58 I only just now realized this is based on |
|
Got a new error while building @slayton58's commit: |
|
Have you pulled the latest version of the code? I’m using CUDA 6.5, gcc 4.6.3 and cuDNN v2 RC2 and it compiles without issue on my machine
|
|
@slayton58 I pulled slayton58@bb00447. I am using
|
|
Replaced by #1854 to dev. |
Modify cuDNN layers to use the R2 API for cuDNN