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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
