Remove dependencies on FP16 support status. #2403
Conversation
myungjoo
requested review from
jijoongmoon,
again4you,
jaeyun-jung,
leemgs,
wooksong,
helloahn,
kparichay,
gichan-jang,
anyj0527,
zhoonit and
a team
as code owners
|
📝 TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #2403. Please a submit 1commit/1PR (one commit per one PR) policy to get comments quickly from reviewers. Your PR must pass all verificiation processes of cibot before starting a review process from reviewers. If you are new member to join this project, please read manuals in documentation folder and wiki page. In order to monitor a progress status of your PR in more detail, visit http://ci.nnstreamer.ai/. |
| @@ -178,11 +178,9 @@ calc_iou(nntrainer::Tensor &bbox1_x1, nntrainer::Tensor &bbox1_y1, | |||
| if (type_intersection_width == ml::train::TensorDim::DataType::FP32) { | |||
| intersection_width.apply_i<float>(nntrainer::ActiFunc::relu<float>); | |||
| } else if (type_intersection_width == ml::train::TensorDim::DataType::FP16) { | |||
| #ifdef ENABLE_FP16 | |||
| if (!is_fp16_enabled()) | |||
| throw std::runtime_error("Not supported data type: fp16"); | |||
There was a problem hiding this comment.
or maybe we can remove these checks and let it be cheked at apply_i() or its underlying function.
|
|
cibot:
myungjoo
force-pushed
the
codeclean/fp16
branch
from
0711bc2 to
cf20b63
Compare
myungjoo
force-pushed
the
codeclean/fp16
branch
from
cf20b63 to
b39f102
Compare
|
|
myungjoo
force-pushed
the
codeclean/fp16
branch
from
b39f102 to
844710b
Compare
myungjoo
force-pushed
the
codeclean/fp16
branch
from
844710b to
97bb166
Compare
|
|
|
|
myungjoo
force-pushed
the
codeclean/fp16
branch
from
97bb166 to
8af9d00
Compare
|
|
|
|
myungjoo
force-pushed
the
codeclean/fp16
branch
from
8af9d00 to
a658336
Compare
|
|
|
|
api/ccapi/include/tensor_dim.h
Outdated
|
|
||
| #ifdef ENABLE_FP16 | ||
| #ifdef USE__FP16 | ||
| #define _FP16 __fp16 | ||
| #else | ||
| #define _FP16 _Float16 | ||
| #endif | ||
| #else /* !ENABLE_FP16 */ | ||
| #define _FP16 uint16_t /* Keep the FP16 programming interface, but don't allow using it in run-time */ |
There was a problem hiding this comment.
after this fix, I think we'll need to add test cases to certify every part is well covered.
In the case of tensors, such as follows.
Tensor fp32_tensor(..., Tdatatype::FP32);
EXPECT_THROW({ fp32_tensor.getValue<FP16>(1); }, std::invalid_argument);
EXPECT_THROW(
{
fp32_tensor.apply(
(std::function<_FP16(_FP16)>)[&](_FP16 in) { return idx++ % 2; });
},
std::invalid_argument);
previously, there was no need to check because _FP16 cannot be used when fp16 is not enabled.
if everyone agrees, I'll work on adding test cases at the tensor level.
myungjoo
force-pushed
the
codeclean/fp16
branch
from
a658336 to
6f56848
Compare
|
|
|
|
_FP16 is the macro to unify different fp16 typenames across different architectures or libraries. Note that util_simd.cpp has the correct name (_FP16) while the header has the incorrect naming (__fp16). Although this does not break the build or execution, this is not good for readability and dependency clean-ups. CC: @skykongkong8 Signed-off-by: MyungJoo Ham <[email protected]>
myungjoo
force-pushed
the
codeclean/fp16
branch
from
6f56848 to
015380d
Compare
|
|
myungjoo
force-pushed
the
codeclean/fp16
branch
2 times, most recently
from
d5f28b8 to
559e50b
Compare
Most modules should NOT depend on the existnace of FP16 support. Let a single (or minimal) entity (a single class?) know and handle it. This is a suggestion commit that shows an example how to refactor it. Most modules are not yet touched with this commit. Signed-off-by: MyungJoo Ham <[email protected]>
myungjoo
force-pushed
the
codeclean/fp16
branch
from
559e50b to
44e38f4
Compare
|
|
|
We can consider this PR proposed for better readability and maintainability. We are in the middle of refactoring Tensor, BLAS, and Multi-Type Support and the codes will be changed a lot. |
This allows writing FP16-independent code without breaking the current code. This is a non-invasive subset of nnstreamer#2403 to address the concern of nnstreamer#2403 (comment) I believe nntrainer can use this during the refactoring so that we do not introduce additional code divergences. Signed-off-by: MyungJoo Ham <[email protected]>
jijoongmoon
requested review from
lhs8928,
songgot,
DonghakPark,
baek2sm,
skykongkong8 and
EunjuYang
as code owners
Most modules should NOT depend on the existnace of FP16 support. Let a single (or minimal) entity (a single class?) know and handle it.
This is a suggestion commit that shows an example how to refactor it. Most modules are not yet touched with this commit.
CC: @skykongkong8 (I've seen some #if/#endif in your commits.)