[Openmp-commits] [PATCH] D60223: [OpenMP][libomptarget] Enable requires flags for target libraries. Support for target link variables when unified memory is enabled

[Alexandre Eichenberger via Phabricator via Openmp-commits]

AlexEichenberger requested changes to this revision.
AlexEichenberger added a comment.
This revision now requires changes to proceed.

See suggested changes, should be pretty straightforward to do, let me know if you need help



================
Comment at: libomptarget/include/omptarget.h:63
 
+enum OpenMPOffloadingRequiresDirFlags {
+  /// reverse_offload clause.
----------------
I wonder if it would not be wise to add an "undefined" flag. Initially, the internal state would be in undefined state. Then when the RT receives a "register_requires", one of two actions can happen.

If the RT state is in undefined state, it looks if the new requires are compatible with the current device; if yes, it accepts it, if not it either silently "degrade" the support to something supported, or abort.

If the RT state is not in undefined state, then it looks if the new require is compatible with the prior "register_required" emitted state. If incompatibilities are found, it abort.

Alternatively, one can add an additional bit in the RT state that indicates if one or more register requires have been reported. That second approach is cleaner but need one more variable. 


================
Comment at: libomptarget/plugins/cuda/src/rtl.cpp:234
+
+    RequiresFlags = 0;
   }
----------------
See comment above, either set to unitialized, or let the state 0, and add additional variable to state "not initialized yet".




================
Comment at: libomptarget/src/rtl.cpp:193
+void RTLsTy::RegisterRequires(int64_t flags) {
+  RequiresFlags = flags;
+}
----------------
compatibility test required... minimally: accept if not previously initialized; refuse if different for any target related flags


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D60223