search

apache / mxnet

Lightweight, Portable, Flexible Distributed/Mobile Deep Learning with Dynamic, Mutation-aware Dataflow Dep Scheduler; for Python, R, Julia, Scala, Go, Javascript and more
https://mxnet.apache.org
Apache License 2.0
20.78k stars 6.79k forks source link

reinterpret_cast in transformer.cu needs review #19005

Open DickJC123 opened 4 years ago

DickJC123 commented 4 years ago

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 ?

leezu commented 4 years ago

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.