[Openmp-commits] [PATCH] D136103: OpenMP asynchronous memory copy support

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

In D136103#3876097 <https://reviews.llvm.org/D136103#3876097>, @jz10 wrote:

> Thanks Johannes and Shilei
>
> 1. '385-387 are the same as in omp_target_memcpy_async. Can we also not duplicate those lines?'
>
> I put the common code (i.e.helper task creation) into another static function

I feel like I'm missing something. As said before, all but 3 lines are identical in these two functions.
Now you created a helper for 1/3 of those identical lines but left the other 2/3 being duplicated. Could
you elaborate why?

> 2. 'In this code there are also various places with variables not named according to the style guide'
>
> fixed, please check if there's remaining issues
>
> 3. 'The problem with the int32 Flags I mentioned already. '
>
> The flag variable was defined as 'kmp_int32', since its consumer '__kmpc_omp_target_task_alloc' needs  'kmp_int32' type as input. the type cast to 'kmp_tasking_flags_t' is to set the 'hidden_helper' bit. So it seems that there's no better option for us. Please let me know your suggestion.

Yeah,.. the API is broken. Let's keep it as is. Apologies to have brought it up.

> 4. "Can we put all KMP related code into a separate header"
>
> we used both kmp relevent data structure/types and APIs, so should I wrap all those relevant code into several tool functions and put them into separate header file?
>
> 5. 'aving a variable suffix with _ is generally not a good coding style'
>
> fixed




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136103/new/

https://reviews.llvm.org/D136103