haojin2 commented on a change in pull request #16699: Mixed data type binary ops URL: https://github.com/apache/incubator-mxnet/pull/16699#discussion_r342324211
########## File path: src/operator/mshadow_op.h ########## @@ -194,6 +194,100 @@ MXNET_BINARY_MATH_OP_NC(right, b); MXNET_BINARY_MATH_OP_NC(mul, a * b); +#ifndef _WIN32 Review comment: It was due to the `C1002: out of heap space` error we've encountered many times. We're not generating more kernels(code) on windows to prevent hitting that error on windows machines. If you still think windows is excluded I would only think you've not given the code changes a complete look: 1. There's also some parts that we have `#ifdef _WIN32` such as: https://github.com/apache/incubator-mxnet/pull/16699/files#diff-c383124e9cb87f51ac456a96b799615aR73 2. We also have parts that have `#else` blocks such as: https://github.com/apache/incubator-mxnet/pull/16699/files#diff-c383124e9cb87f51ac456a96b799615aR73 This is indeed a workaround for an issue that we could not solve on our own. I've also tried with upgrading vs compiler locally and it does not get this issue out of our way so that's why we have different impls for this same feature, otherwise we only have to drop this new feature for windows users. It's good to be eager to learn, but IMHO blocking a PR without a complete look and a very solid reason is not a good (nor polite) way for demonstrating your eagerness. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
