[SYCL][ESIMD] Convert esimd operator to friend method. #2994
Conversation
cmc-rep
force-pushed
the
cmc-rep-change
branch
from
ebd2dc0 to
7d3ee31
Compare
| @@ -184,30 +184,26 @@ template <typename Ty, int N> class simd { | |||
| // {/quote} | |||
| // * if not different, then auto should not be used | |||
| #define DEF_BINOP(BINOP, OPASSIGN) \ | |||
| auto operator BINOP(const simd &RHS) const { \ | |||
| ESIMD_INLINE friend auto operator BINOP(const simd &x, const simd &y) { \ | |||
There was a problem hiding this comment.
Please make parameter naming consistent with other friends (here and below):
| ESIMD_INLINE friend auto operator BINOP(const simd &x, const simd &y) { \ | |
| ESIMD_INLINE friend auto operator BINOP(const simd &Y, const simd &Y) { \ |
|
I am waiting for the test result. It seems taking forever |
cmc-rep
force-pushed
the
cmc-rep-change
branch
2 times, most recently
from
85bc19b to
612d783
Compare
|
Updated change |
| auto R = data() RELOP RHS.data(); \ | ||
| ESIMD_INLINE friend simd<uint16_t, N> operator RELOP(const simd &X, \ | ||
| const simd &Y) { \ | ||
| auto R = X.data() RELOP Y.data(); \ | ||
| mask_type_t<N> M(1); \ |
There was a problem hiding this comment.
Nit (maybe for another patch): since M is all ones, isn't M & convert<mask_type_t<N>>(R) equivalent to convert<mask_type_t<N>>(R) thus making M redundant?
There was a problem hiding this comment.
M(1) is not part of my change. Not sure why it is written this way. I would be cautious to make unnecessary change
There was a problem hiding this comment.
Yes, I see that it is not part of this change, that's why it is just a nit for another patch. I thought you wrote this part.
| ESIMD_INLINE friend auto operator BINOP(const simd_view &X, \ | ||
| const simd_view &Y) { \ | ||
| return (X BINOP Y.read()); \ | ||
| } \ | ||
| simd_view &operator OPASSIGN(const value_type &RHS) { \ |
There was a problem hiding this comment.
is value_type BINOP value_type combination missing?
There was a problem hiding this comment.
Cannot have that, cause ambiguous with the friend method definition in simd class
| } \ | ||
| ESIMD_INLINE friend simd<uint16_t, length> operator RELOP( \ | ||
| const simd_view &X, const simd_view &Y) { \ | ||
| return (X RELOP Y.read()); \ | ||
| } |
There was a problem hiding this comment.
is value_type RELOP value_type combination missing?
There was a problem hiding this comment.
Cannot have that, cause ambiguous with the friend method definition in simd class
|
@kbobrovs please help me merge this PR. Somehow I am not authorized to merge |
| @@ -389,7 +429,7 @@ template <typename BaseTy, typename RegionTy> class simd_view { | |||
| // - std::pair<top_region_type, base_region_type> | |||
| // | |||
| RegionTy M_region; | |||
| }; | |||
| }; // namespace gpu | |||
There was a problem hiding this comment.
Missed this one in the initial review. Please fix.
| }; // namespace gpu | |
| }; |
There was a problem hiding this comment.
I will work on some change to the same file again. Could I fix it in the next change. So I can move on working on the next change
There was a problem hiding this comment.
@kbobrovs, fix it. Please review and merge timely.
Signed-off-by: Gang Y Chen <[email protected]>
cmc-rep
force-pushed
the
cmc-rep-change
branch
from
612d783 to
f8c72fa
Compare
|
@konst, this PR is ready, please help with merge |
|
@kbobrovs this PR is ready, please help with merge |
bader
changed the title
Signed-off-by: Gang Y Chen [email protected]