[refactor] Replace std::map with switch case statements in lang_util.cpp #1755
Conversation
There was a problem hiding this comment.
Very cool!!! I believe your strategy mostly works. For the special cases, let's use a case instead of macros :-)
Please go ahead and refactor more functions. Feel free to ask me to take another look if there's anything you are unsure of during the refactoring.
Thanks again for contributing! Looking forward to having this PR merged :-)
taichi/lang_util.cpp
Outdated
| case DataType::i: \ | ||
| return sizeof(j); | ||
|
|
||
| REGISTER_DATA_TYPE(DataType::f16, 2); |
There was a problem hiding this comment.
Actually, sizeof(2) gives you 4 since 2 is treated as int32. Since this option is not regular, maybe we can use an explicit case DataType::f16: return 2 instead of the macro :-)
taichi/lang_util.cpp
Outdated
| type_sizes[DataType::gen] = 0; | ||
| type_sizes[DataType::unknown] = -1; | ||
| REGISTER_DATA_TYPE(DataType::gen, 0); |
taichi/lang_util.cpp
Outdated
| default: //unknown | ||
| return -1; |
There was a problem hiding this comment.
Let's treat unknown in a switch branch, and use TI_NOT_IMPLEMENTED in the default case: it's not impossible in C++ that someone triggered undefined a behavior and DataType's value equals non of its options. We should fail loudly in that case.
|
I've refactored the functions and put TI_NOT_IMPLEMENTED in the default blocks, and corrected the sizing errors. Let me know if anything else needs work. Thank you! |
|
Very cool! Could you fix the compilation errors? https://github.com/taichi-dev/taichi/runs/1018642631#step:5:200 It's highly recommended to have a local Taichi built from source: https://taichi.readthedocs.io/en/latest/dev_install.html#developer-installation so that you can fix the compilation errors locally. |
lang_util_backup.cpp
Outdated
| @@ -0,0 +1,519 @@ | |||
| // Definitions of utility functions and enums | |||
There was a problem hiding this comment.
Let's move this file to misc/lang_util_backup.cpp to prevent confusion.
yuanming-hu
changed the title
|
@yuanming-hu The switch-case statement doesn't work for pairs of DataTypes, so should I leave the promoted_type method as-is? |
Sure, sounds good to me! We can leave it to a future PR. Thank you. |
Codecov Report
@@ Coverage Diff @@
## master #1755 +/- ##
==========================================
- Coverage 43.51% 43.43% -0.08%
==========================================
Files 44 44
Lines 6023 6011 -12
Branches 1078 1075 -3
==========================================
- Hits 2621 2611 -10
+ Misses 3248 3246 -2
Partials 154 154
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Awesome! I believe we are mostly done. Please apply the suggestions if those make sense - then I can merge this once the tests pass. Currently, the tests fail because of the deletion of bit_shl and bit_shr.
Thank you so much for contributing! This is a very very helpful refactoring to have since we will have parallel compilation very soon.
taichi/lang_util.cpp
Outdated
|
|
||
| REGISTER_DATA_TYPE(f32, float32); | ||
| REGISTER_DATA_TYPE(f64, float64); | ||
| // REGISTER_DATA_TYPE(i8, bool); // duplicate value |
There was a problem hiding this comment.
| // REGISTER_DATA_TYPE(i8, bool); // duplicate value |
Nice catch! Let's remove this line then :-)
taichi/lang_util.cpp
Outdated
| REGISTER_TYPE(bit_shl, <<); | ||
| REGISTER_TYPE(bit_sar, >>); |
There was a problem hiding this comment.
I believe we still need these lines?
Related issue = #1750
[Click here for the format server]
Hello, I tried refactoring the first two functions. Once these changes look good, I'll apply them to every function with std::map.