Add Majel's Dim to Paddle #2202
Conversation
|
|
||
| std::vector<int> v3; | ||
|
|
||
| assert(v1.size() == v2.size()); |
There was a problem hiding this comment.
Why not use the MAJEL_ASSERT directly?
There was a problem hiding this comment.
According to util.h, MAJEL_ASSERT is available only in kernel code of Apple GPU. In other cases, we should use assert instead.
paddle/majel/ddim.cc
Outdated
| @@ -0,0 +1,222 @@ | |||
| #include <majel/ddim.h> | |||
There was a problem hiding this comment.
#include "paddle/majel/ddim.h"
-
Please use double-quotes instead of angle-brackets, which is for system header files. More details at here: https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
-
Please make sure to use the full #include path. This makes sure that there is only one way to include each header file.
paddle/majel/test/CMakeLists.txt
Outdated
| @@ -2,6 +2,11 @@ cc_test(place_test | |||
| SRCS place_test.cc | |||
| DEPS majel) | |||
|
|
|||
| cc_test(ddim_test | |||
There was a problem hiding this comment.
In the next PR, we need to move all unit tests, e.g., place_test.cc and ddim_test.cc, to one folder level above. According to the convention here https://google.github.io/styleguide/cppguide.html#File_Names, we want three files of each module xxx
- xxx.h
- xxx.{cc,cu}
- xxx_test.{cc,cu}
in the same place, so to ease code browsing.
I created an issue #2209 to remind us about this.
There was a problem hiding this comment.
ok, to avoid clutter, I'm going to keep the folder structure unchanged temporarily and wait for the adjust in next PR.
paddle/majel/util.h
Outdated
| namespace detail { | ||
|
|
||
| inline int div_up(int x, int y) { return (x + y - 1) / y; } | ||
| } |
There was a problem hiding this comment.
There should be an empty line between L37 and L38, because you put an empty in the corresponding place at L33.
paddle/majel/util.h
Outdated
|
|
||
| inline int div_up(int x, int y) { return (x + y - 1) / y; } | ||
| } | ||
| } |
There was a problem hiding this comment.
Please follow https://google.github.io/styleguide/cppguide.html#Namespaces to add comments after the closing parenthesis of namespace:
} // namespace detail
} // namespace majel
paddle/majel/CMakeLists.txt
Outdated
| @@ -1,4 +1,4 @@ | |||
| cc_library(majel SRCS place.cc) | |||
| cc_library(majel SRCS place.cc ddim.cc) | |||
There was a problem hiding this comment.
According to our design, I believe that this line should be
cc_library(place SRCS place.cc)
cc_library(dim SRCS dim.cc)
cc_library(ddim SRCS ddim.cc DEPS dim)There was a problem hiding this comment.
Maybe we need to add an optional OBJECT, because CMake add_library(name OBJECT source) can generate *.o
There was a problem hiding this comment.
done.
But Dim is only a part of DDim, it is not an independent module and only has a dim.h file. So maybe these lines should be
cc_library(place SRCS place.cc)
cc_library(ddim SRCS ddim.cc)
paddle/majel/dim.h
Outdated
| #include <type_traits> | ||
|
|
||
| #include "majel/hostdevice.h" | ||
| #include "majel/util.h" |
There was a problem hiding this comment.
How about moving this header file majel/util.h into majel/detail/assert.h? Please see my comments in this file for more details.
There was a problem hiding this comment.
done. And I moved hostdevice.h into folder detail as well.
paddle/majel/util.h
Outdated
| namespace majel { | ||
| namespace detail { | ||
|
|
||
| inline int div_up(int x, int y) { return (x + y - 1) / y; } |
There was a problem hiding this comment.
This function div_up is not used in the ported part of Majel. Also, I checked that it's used in the following files of Majel:
$ for i in $(du -a | grep '\.h$' | cut -f 2); do if grep 'div_up' $i > /dev/null; then echo $i; fi; done
./detail/bsxfun.h
./detail/util.h
./gpu/detail/gather.h
./gpu/detail/gather_with_indices.h
./gpu/detail/ops.h
./gpu/detail/quantize_ops.h
./gpu/detail/reduce.h
./gpu/detail/row_conv_ops.h
./gpu/detail/scatter_with_indices.h
I am not sure if we will need it soon. How about we don't port it right now? If so, we can name this header file paddle/majel/detial/assert.
There was a problem hiding this comment.
Function div_up has been removed. But I feel it's better to keep the file's name util.h. Because in this file, we #include the system's assert.h. It may be confusing if both of them are named assert.h.
Or maybe cuda_assert.h is a better name? For it is only used in cuda kernel code.
| } // namespace | ||
| // Static access to constant Dim | ||
| template <int i, int l> | ||
| HOSTDEVICE int get(const Dim<l>& d) { |
There was a problem hiding this comment.
It seems that get and operator[] do the same thing? Do we need both?
There was a problem hiding this comment.
They indeed do the same thing. I'm not sure whether both of them are required by higher level structs. However, I guess offering more interfaces is unlikely to be bad, is it?
There was a problem hiding this comment.
You are right. I asked Greg. He confirmed that they are doing the same thing. :-)
Dim is used in Majel to describe array size or element index. It is the foundation of higher level structs like Array.
Implements of Dim are contained in dim.h, ddim.h and ddim.cc.
hostdevice.h and util.h are for macro definition.