-
Notifications
You must be signed in to change notification settings - Fork 242
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
[GTests] Convert TensorOps CTest to GTest #3424
Conversation
test/gtest/ternary_tensor_ops.cpp
Outdated
tensor<T> tensorGPU = std::move(CalculateOnGPU()); | ||
tensor<T> tensorCPU = std::move(CalculateOnCPU()); |
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.
std::move
is not required because of mandatory RVO. RVO guarantees zero cost in-place object creation without even copying or moving anything.- No need to mimic old approach, since it's kind of weird and incredibly inefficient. See Move tensor_set/scale/cast/copy to gtest #3416
test/gtest/ternary_tensor_ops.cpp
Outdated
auto r = tensorC; | ||
r.data = handle.Read<T>(c_dev, r.data.size()); |
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.
That's inefficient, you copied tensorC
and override it right away with handle.Read
, which... doubles the memory requirements by creating new vector.
See #3416 how it can be done in more efficient way.
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 but a small 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
LGTM |
Test is refactored.
Three types are supported: float, double and half. For example, I8 and BF16 are not supported even in original CTest.
Test is renamed to ternary_tensor_ops as was requested by @CAHEK7