[Openmp-commits] [PATCH] D63106: [OpenMP][libomptarget] Add support for declare target to clause under unified memory

[George Rokos via Phabricator via Openmp-commits]

grokos added inline comments.


================
Comment at: libomptarget/plugins/cuda/src/rtl.cpp:452-453
       if (DeviceInfo.RequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY &&
-          e->flags & OMP_DECLARE_TARGET_LINK) {
+          (e->flags & OMP_DECLARE_TARGET_LINK ||
+           e->flags == OMP_DECLARE_TARGET_TO)) {
         // If unified memory is present any target link variables
----------------
gtbercea wrote:
> grokos wrote:
> > jcownie wrote:
> > > gtbercea wrote:
> > > > grokos wrote:
> > > > > gtbercea wrote:
> > > > > > grokos wrote:
> > > > > > > Maybe this OR is redundant; it always evaluates to true. Since the symbol we are processing has size!=0 it is a variable and variables are always either "to" or "link", there are no other possibilities (at least now, maybe in the future more attributes will be added).
> > > > > > So I could remove the attribute check and just have:
> > > > > > 
> > > > > > ```
> > > > > > if (DeviceInfo.RequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY) {..} 
> > > > > > ```
> > > > > > Would that work?
> > > > > > 
> > > > > > The reason I included this explicit check (even if it evaluates to true always) is that if in the future this is extended with other attributes then this check will not automatically work for that. I think this disambiguates the cases that are supported today and guard against this path being taken unintentionally.
> > > > > >  
> > > > > Yes, that's what I thought as well. Let's leave the check there, it makes the implementation more future-proof.
> > > > Awesome. I'll leave it as is for now then.
> > > The code which is checking the flags looks a bit weird. One test is done with & (suggesting these are bit flags which could be or-ed together), the other with == suggesting this is a field full of mutually exclusive enumerations.
> > This is because actually "TO" is not a flag, it's the complete absence of flags (0x00). If something has been "declared target" and is not a ctor/dtor or a linked variable then it is "TO".
> > 
> > I don't like this fact either - if we want to be 100% correct we should introduce a dedicated flag for "TO" but that involves changes in the compiler/library interface and is definitely not part of this patch. We can pursue such a patch anyway if there are strong arguments that warrant the effort.
> I've move the checks into a comment and simplified the condition.
This is what I had in mind when I posted the first comment. It looks better now!


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D63106