ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.
LG
================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9470-9471
+ MappedVarSet.insert(CI->getCapturedVar()->getCanonicalDecl());
+ else
+ MappedVarSet.insert(nullptr);
if (CurBasePointers.empty())
----------------
jdenny wrote:
> 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.
Ah, I see, I missed the previous line.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83922/new/
https://reviews.llvm.org/D83922
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
