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

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

jdenny marked an inline comment as done.
jdenny added inline comments.


================
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:
> > > 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.)
> `MappedVarSet` is a new variable, right? It is used only in 2 places: here, where you add elements to this set, and in `generateAllInfo` where you have a check:
> ```
> if (SkipVarSet.count(VD))
>             return;
> ```
> I don't see other places where it is used. And I don't see places where you check for the presence of `nullptr` in this set. That's why I think you don't need to add it, if you don't check for its presence later.
> MappedVarSet is a new variable, right?

Right.

> It is used only in 2 places: here, where you add elements to this set, and in generateAllInfo where you have a check:
> 
> ```
> if (SkipVarSet.count(VD))
>             return;
> ```
> I don't see other places where it is used.

That's all.

> And I don't see places where you check for the presence of nullptr in this set. That's why I think you don't need to add it, if you don't check for its presence later.

The `SkipVarSet.count(VD)` you mentioned checks for presence of any key in order to avoid creating duplicate map entries.  `nullptr` is one such key.  It happens to indicate `this`.

For comparison, look inside `generateInfoForCapture` where the following code establishes that `nullptr` is the key for `this`:

```
    const ValueDecl *VD = Cap->capturesThis()
                              ? nullptr
                              : Cap->getCapturedVar()->getCanonicalDecl();
```

After that, does `generateInfoForCapture` ever check for `VD == nullptr`?  I don't see where it does.  But it does use `VD`, which might be `nulltpr`, as a decl components key in the following code:

```
      for (const auto L : C->decl_component_lists(VD)) {
```

Likewise, my code is also using a `VD` that might be `nullptr` as a key in `SkipVarSet`.  I don't have to special case `nullptr` at this point.  It's just another key.


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

https://reviews.llvm.org/D83922