[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:7948-7949
+                       MapFlagsArrayTy &Types,
+                       const llvm::DenseSet<const ValueDecl *> &SkipVarSet =
+                           llvm::DenseSet<const ValueDecl *>()) const {
     // We have to process the component lists that relate with the same
----------------
ABataev wrote:
> Use `llvm::DenseSet<CanonicalDeclPtr<const ValueDecl>> &SkipVarSet`.
Thanks for the suggestion.

`CanonicalDeclPtr<const ValueDecl>` appears to be unusable.  The constructor (in `Redeclarable.h`) tries to initialize the `const ValueDecl *Ptr` member with the `Decl *` returned by `getCanonicalDecl`, producing a type mismatch.

I went with `CanonicalDeclPtr<const Decl>` instead.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9468-9469
                                          CurSizes, CurMapTypes, PartialStruct);
+        if (!CI->capturesThis())
+          MappedVarSet.insert(CI->getCapturedVar()->getCanonicalDecl());
+        else
----------------
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?


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9470-9471
+          MappedVarSet.insert(CI->getCapturedVar()->getCanonicalDecl());
+        else
+          MappedVarSet.insert(nullptr);
         if (CurBasePointers.empty())
----------------
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]
```


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

https://reviews.llvm.org/D83922