-
Notifications
You must be signed in to change notification settings - Fork 5.9k
merge CMakeList.txt manual #35378
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
merge CMakeList.txt manual #35378
Conversation
|
Thanks for your contribution! |
XieYunshen
left a comment
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.
LGTM
由于机器原因,jetson上的单测执行时间较长
| #ifdef __HIPCC__ | ||
| // HIP will throw core dump when threads > 256 | ||
| constexpr int num_threads = 256; | ||
| #elif WITH_NV_JETSON |
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.
这个有测试过么? __HIPCC__应该是为支持华为硬件的宏,NVIDA的gpu不会运行到这个分支(WITH_GPU的时候不会编译HIP代码)。
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.
这个有测试过么? __HIPCC__应该是为支持华为硬件的宏,NVIDA的gpu不会运行到这个分支(WITH_GPU的时候不会编译HIP代码)。
经确认逻辑正确
| dim3 threads(1024, 1); | ||
| int thread_num = 1024; | ||
| #ifdef WITH_NV_JETSON | ||
| // platform::ChangeThreadNum(context, &thread_num); |
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.
此处为何没有保持风格一致,也使用函数调用?
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.
此处为何没有保持风格一致,也使用函数调用?
这个op比较特殊,在调用的时候不需要传入设备上下文,所以无法根据运行时的实际设备调整线程数,只能在编译的时候指定,如果修改op的参数定义的话,涉及到的调用位置都要补充获取设备上下文参数的逻辑,影响面比较大。这样简单的修改话会影响agx和nx, 不过我让王也测试过,之前的模型性能没有下降,反而略微提升,所以目前看来可以这样指定。
shangzhizhou
left a comment
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.
LGTM
Superjomn
left a comment
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.
LGTM
* merge CMakeList.txt manual * add platform for changethreadnum * repair some bugs according to make error * do nothing just flush CI * forget change thread num * add inplace_atol param for check_output_with_place * Windows * std:min and std::max should be change because of windows
* merge CMakeList.txt manual * add platform for changethreadnum * repair some bugs according to make error * do nothing just flush CI * forget change thread num * add inplace_atol param for check_output_with_place * Windows * std:min and std::max should be change because of windows
PR types
Others
PR changes
Others
Describe
jetson-op-test