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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits