jdoerfert added a comment.
We need a runtime test.
Synchronizing the asyn info object and signal out dependences will happen in
the next revision.
So will minor edits as requested.
I commented below on some things.
================
Comment at: openmp/libomptarget/src/interop.cpp:13
+static omp_interop_rc_t
+__kmpc_interop_get_property_err_type(omp_interop_property_t property) {
+ switch (property) {
----------------
tianshilei1992 wrote:
> tianshilei1992 wrote:
> > tianshilei1992 wrote:
> > > There are some conventions in current `libomptarget`.
> > > 1. If a function is internal, use LLVM code style.
> > > 2. If a function is exported and exposed to compiler, it should be
> > > `extern "C"` and use code style similar to existing functions whose name
> > > prefix with `__tgt`.
> > >
> > > So basically, if these functions are only visible to this file, please
> > > format it with LLVM code style, and use anonymous name space.
> > I mean, this function doesn't have to start with `__tgt` because it is
> > internal. Functions starting with `__tgt` are usually interface functions.
> > From my perspective, it is better to name it as `getPropertyErrorType`,
> > a.k.a. to use LLVM code style.
> Looks better. Can you check https://llvm.org/docs/CodingStandards.html for
> the code style?
It's unclear given the mixed nature. Let's go with this and figure out if we
need to rename vars later or not.
================
Comment at: openmp/libomptarget/src/interop.cpp:198-201
+ if (interop_type == kmp_interop_type_tasksync) {
+ __kmpc_omp_wait_deps(loc_ref, gtid, ndeps, dep_list, ndeps_noalias,
+ noalias_dep_list);
+ }
----------------
RaviNarayanaswamy wrote:
> Interop object does not wait for any task from libomp.
> Init just initializes the interop object.
> The initialization of interop object should be done by the plugin as only
> the plugin knows what properties are supported
> Interop object does not wait for any task from libomp.
I don't know why you think we would not wait for libomp tasks. If we have
dependences we need to wait for them.
> The initialization of interop object should be done by the plugin as only the
> plugin knows what properties are supported.
It is, below. This is the generic part that then redirects to the plugin.
================
Comment at: openmp/libomptarget/src/interop.cpp:260-263
+ if (interop_val->interop_type == kmp_interop_type_tasksync) {
+ __kmpc_omp_wait_deps(loc_ref, gtid, ndeps, dep_list, ndeps_noalias,
+ noalias_dep_list);
+ }
----------------
RaviNarayanaswamy wrote:
> You don't wait for the omp tasks. Need to flush the queue associated with
> the interop through the plugin
Waiting for omp task is necessary and the above should do it. Flushing and
signaling out dependences will be added in the next revision.
================
Comment at: openmp/runtime/src/kmp_ftn_entry.h:1449-1494
+/// TODO: Include the `omp.h` of the current build
+/* OpenMP 5.1 interop */
+typedef intptr_t omp_intptr_t;
+
+/* 0..omp_get_num_interop_properties()-1 are reserved for
implementation-defined
+ * properties */
+typedef enum omp_interop_property {
----------------
RaviNarayanaswamy wrote:
> Why do you have all this in openmp/runtime. Openmp should call libomptarget
> to get interop properties. if libomptarget is not loaded it should return 0
That is exactly what this code does, no? Look for libomptarget and redirect to
that one, otherwise return 0.
Unsure I see your point. FWIW, this is copied from other functions we duplicate
in libomp but that actually are part of libomptarget.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106674/new/
https://reviews.llvm.org/D106674
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
