[GitHub] [incubator-mxnet] DickJC123 opened a new issue #19005: reinterpret_cast in transformer.cu needs review

[GitBox <gi...@apache.org>]

DickJC123 opened a new issue #19005:
URL: https://github.com/apache/incubator-mxnet/issues/19005


   ## Description
   @leezu 
   
   Could you review the use of reinterpret_cast in the following code snippet from a PR of yours:
   https://github.com/apache/incubator-mxnet/blob/3c4ac19daa3e645d918692b864ea19640f7e0314/src/operator/contrib/transformer.cu#L104-L130
   
   The alpha and beta values are floats, so reinterpreting their addresses as pointers to a different primitive type is incorrect I feel.  In "true fp16 mode", the types of these constants match the fundamental DType.  While the variable naming could be better, use of &trueFP16_alpha and &trueFP16_beta might correct the issue.  Testing of this code path must be lacking.
   
   Thoughts also @MoisesHer ?
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-mxnet] leezu commented on issue #19005: reinterpret_cast in transformer.cu needs review

[GitBox <gi...@apache.org>]

leezu commented on issue #19005:
URL: https://github.com/apache/incubator-mxnet/issues/19005#issuecomment-679403045


   Good catch. This code only runs on cuda arch < 5 and the affected integration tests (in https://github.com/aws/deep-learning-containers) seem to only test the float test case.


----------------------------------------------------------------
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:
users@infra.apache.org