[PATCH] D83922: [OpenMP] Fix map clause for unused var: don't ignore it

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D83922#2156567 <https://reviews.llvm.org/D83922#2156567>, @ABataev wrote:

> In D83922#2156510 <https://reviews.llvm.org/D83922#2156510>, @jdenny wrote:
>
> > In D83922#2155749 <https://reviews.llvm.org/D83922#2155749>, @ABataev wrote:
> >
> > > I would add checks for mapping of `declare target to/link` vars and checked if they work in runtime.
> >
> >
> > There are existing codegen tests for that, and they don't seem to be affected by this patch.  This patch only addresses the case where a `map` clause is specified for an unused variable.  Is there another behavior this patch might impact?
>
>
> What I mean is the explicit mapping for `declare target to/link` variables. The variable is marked as declare to and then mapped. Do we have the tests for something like this?


I didn't find tests for the case where a variable has a `declare target` and a `map` clause.  I can add some to show that this patch doesn't change the generated map types.

In D83922#2156606 <https://reviews.llvm.org/D83922#2156606>, @ABataev wrote:

> Check, if it fixed https://bugs.llvm.org/show_bug.cgi?id=46012


This patch doesn't affect the example in that bug report as far as I can tell.



================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9470-9471
+          MappedVarSet.insert(CI->getCapturedVar()->getCanonicalDecl());
+        else
+          MappedVarSet.insert(nullptr);
         if (CurBasePointers.empty())
----------------
ABataev wrote:
> jdenny wrote:
> > ABataev wrote:
> > > No need to insert `nullptr` here, it is not used later.
> > Without this `nulltpr` insertion, many tests fail because of duplicate map entries.  Independently of my patch, `nulltptr` is used to indicate `this` capture (see the `generateInfoForCapture` implementation) .
> > 
> > For example:
> > 
> > ```
> > struct S {
> >   int i;
> >   void foo() {
> >     #pragma omp target map(i)
> >     i = 5;
> >   }
> > };
> > ```
> > 
> > We should have:
> > 
> > ```
> > @.offload_maptypes = private unnamed_addr constant [2 x i64] [i64 32, i64 281474976710659]
> > ```
> > 
> > Without the `nullptr` insertion, we have:
> > 
> > ```
> > @.offload_maptypes = private unnamed_addr constant [4 x i64] [i64 32, i64 281474976710659, i64 32, i64 844424930131971]
> > ```
> This is strange, because you don't check for `nullptr`. You only check for `ValueDecl`s, but not capture of `this`.
I'm not sure what code you're referring to when you say "you check".

Both with and without my patch, my understanding is that `nullptr` is a special key that means `this`.  I'm depending on that to avoid generating map entries twice for `this`.

My understanding is based on the way `generateInfoForCapture` works.  If `Cap->capturesThis()`, then `VD = nullptr`.  That `VD` is then used by `C->decl_component_lists(VD)` to find entries for `this` in map clauses.

Unless I'm misreading it, the code that sets `nullptr` for `this` in decl components is `checkMappableExpressionList` in SemaOpenMP.cpp.  The last few lines of that function have a comment to this effect.  (That comment would probably be more useful in a header file somewhere.)


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

https://reviews.llvm.org/D83922