[Openmp-commits] [PATCH] D64218: [OpenMP][NFCI] Cleanup the target synchronization implementation

[Alexey Bataev via Phabricator via Openmp-commits]

ABataev added a comment.

In D64218#1616979 <https://reviews.llvm.org/D64218#1616979>, @jdoerfert wrote:

> In D64218#1616959 <https://reviews.llvm.org/D64218#1616959>, @ABataev wrote:
>
> > In D64218#1616951 <https://reviews.llvm.org/D64218#1616951>, @jdoerfert wrote:
> >
> > > @ABataev I am confused by the comments you make. Could you describe a situation in which we do not execute the same code before and after this patch? Also, the O0/03 inline & constant propagation comment is not clear to me, what is the issue there?
> >
> >
> > I was confused by the names of the template arguments.
>
>
> I'm happy to rename them if we want to go ahead with this `__kmpc_impl_barrier` (potentially without the rest in here). Name suggestions are always welcome.


It would be good to try to gather the common functionality into some kind of class, maybe? Common functionality could be encapsulated into some kind common functionality templated class, while target specific functions could be encapsulated into a target specific class, which could be used as a template argument for common functionality template instantiation.
Something like:

  template <typename TargetT>
  class CommonFunctionality : public TargetT {
    void barrier(....) {
       if (spmd-mode) {
         TargetT::spmd_barrier();
       } else if (num_threads > 1) {
         TargetT::non_spmd_barrier();
       } else {
         TargetT::flush(); 
       }
    }
  };

What do you think about it?
For the template arguments, maybe just do not make it a templated function? Maybe it is better to make  set of simple functions and call them in the complex ones?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64218