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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

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



================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9468-9469
                                          CurSizes, CurMapTypes, PartialStruct);
+        if (!CI->capturesThis())
+          MappedVarSet.insert(CI->getCapturedVar()->getCanonicalDecl());
+        else
----------------
jdenny wrote:
> ABataev wrote:
> > I would check that we capture a variable. We may capture VLA here as well.
> > I would check that we capture a variable. We may capture VLA here as well.
> 
> The `if` on line 9454 above checks for VLAs. This code is in the `else`.
> 
> Similarly, the existing implementation for `generateInfoForCapture`, which is called within this same `else`, doesn't protect against VLAs.  It just checks `capturesThis` as I'm doing there.
> 
> Have I misunderstood?
Ah, yes, here we processed VLAs already. 


================
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:
> > 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`.


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

https://reviews.llvm.org/D83922