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

2020-07-20 Thread Alexey Bataev via Phabricator via cfe-commits 
ABataev added a comment.

In D83922#2162444 , @jdenny wrote:

> In D83922#2162365 , @ABataev wrote:
>
> > Looks like it breaks unified_shared_memory/close_enter_exit.c test. Could 
> > you check this, please?
>
>
> It doesn't fail for me, and I didn't receive a buildbot notification.  Do you 
> have a link?


I saw it failed it in local build, but probably it was caused by a 
misconfiguration, sorry.


Repository:
  rG LLVM Github Monorepo

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

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

2020-07-20 Thread Joel E. Denny via Phabricator via cfe-commits 
jdenny added a comment.

In D83922#2162365 , @ABataev wrote:

> Looks like it breaks unified_shared_memory/close_enter_exit.c test. Could you 
> check this, please?


It doesn't fail for me, and I didn't receive a buildbot notification.  Do you 
have a link?


Repository:
  rG LLVM Github Monorepo

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

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

2020-07-20 Thread Alexey Bataev via Phabricator via cfe-commits 
ABataev added a comment.

Looks like it breaks unified_shared_memory/close_enter_exit.c test. Could check 
this, please?


Repository:
  rG LLVM Github Monorepo

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

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

2020-07-17 Thread Joel E. Denny via Phabricator via cfe-commits 
jdenny marked 3 inline comments as done.
jdenny added a comment.

Thanks for the review.  Will try to push soon.


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

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

2020-07-17 Thread Alexey Bataev via Phabricator via cfe-commits 
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

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

2020-07-17 Thread 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2020-07-17 Thread Alexey Bataev via Phabricator via cfe-commits 
ABataev added inline comments.



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


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

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

2020-07-16 Thread Joel E. Denny via Phabricator via cfe-commits 
jdenny added a comment.

In D83922#2156567 , @ABataev wrote:

> In D83922#2156510 , @jdenny wrote:
>
> > In 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 , @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

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

2020-07-16 Thread 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2020-07-16 Thread Alexey Bataev via Phabricator via cfe-commits 
ABataev added a comment.

In D83922#2156510 , @jdenny wrote:

> In 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?


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

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

2020-07-16 Thread Joel E. Denny via Phabricator via cfe-commits 
jdenny added a comment.

In 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?


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

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

2020-07-16 Thread 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 ,
+   const llvm::DenseSet  =
+   llvm::DenseSet()) const {
 // We have to process the component lists that relate with the same

ABataev wrote:
> Use `llvm::DenseSet> `.
Thanks for the suggestion.

`CanonicalDeclPtr` 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` 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2020-07-16 Thread Joel E. Denny via Phabricator via cfe-commits 
jdenny updated this revision to Diff 278546.
jdenny marked 7 inline comments as done.
jdenny edited the summary of this revision.
jdenny added a comment.

Use `CanonicalDeclPtr`, as suggested.


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

https://reviews.llvm.org/D83922

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/test/OpenMP/target_map_codegen.cpp
  clang/test/OpenMP/target_teams_map_codegen.cpp

Index: clang/test/OpenMP/target_teams_map_codegen.cpp
===
--- clang/test/OpenMP/target_teams_map_codegen.cpp
+++ clang/test/OpenMP/target_teams_map_codegen.cpp
@@ -20,15 +20,16 @@
 #ifndef HEADER
 #define HEADER
 
+// HOST: @[[MAPTYPES_PRIVATE:.offload_maptypes[0-9.]*]] = private {{.*}}constant [2 x i64] [i64 35, i64 35]
 // HOST: @[[MAPTYPES_FIRSTPRIVATE:.offload_maptypes[0-9.]*]] = private {{.*}}constant [2 x i64] [i64 35, i64 35]
 // HOST: @[[MAPTYPES_REDUCTION:.offload_maptypes[0-9.]*]] = private {{.*}}constant [2 x i64] [i64 35, i64 35]
 // HOST: @[[MAPTYPES_FROM:.offload_maptypes[0-9.]*]] = private {{.*}}constant [1 x i64] [i64 34]
 // HOST: @[[MAPTYPES_TO:.offload_maptypes[0-9.]*]] = private {{.*}}constant [1 x i64] [i64 33]
 // HOST: @[[MAPTYPES_ALLOC:.offload_maptypes[0-9.]*]] = private {{.*}}constant [1 x i64] [i64 32]
-// HOST: @[[MAPTYPES_ARRAY_R0:.offload_maptypes[0-9.]*]] = private {{.*}}constant [2 x i64] [i64 35, i64 35]
-// HOST: @[[MAPTYPES_ARRAY_R1:.offload_maptypes[0-9.]*]] = private {{.*}}constant [2 x i64] [i64 33, i64 33]
-// HOST-INT128: @[[MAPTYPES_INT128_R0:.offload_maptypes[0-9.]*]] = private {{.*}}constant [2 x i64] [i64 35, i64 35]
-// HOST-INT128: @[[MAPTYPES_INT128_R1:.offload_maptypes[0-9.]*]] = private {{.*}}constant [2 x i64] [i64 34, i64 34]
+// HOST: @[[MAPTYPES_ARRAY_R0:.offload_maptypes[0-9.]*]] = private {{.*}}constant [3 x i64] [i64 35, i64 35, i64 35]
+// HOST: @[[MAPTYPES_ARRAY_R1:.offload_maptypes[0-9.]*]] = private {{.*}}constant [3 x i64] [i64 33, i64 33, i64 33]
+// HOST-INT128: @[[MAPTYPES_INT128_R0:.offload_maptypes[0-9.]*]] = private {{.*}}constant [3 x i64] [i64 35, i64 35, i64 35]
+// HOST-INT128: @[[MAPTYPES_INT128_R1:.offload_maptypes[0-9.]*]] = private {{.*}}constant [3 x i64] [i64 34, i64 34, i64 34]
 //
 // CHECK: @.omp_offloading.entry_name{{[0-9.]*}} = {{.*}} c"[[OFFLOAD_PRIVATE:__omp_offloading_[^"\\]*mapWithPrivate[^"\\]*]]\00"
 // CHECK: @.omp_offloading.entry_name{{[0-9.]*}} = {{.*}} c"[[OFFLOAD_FIRSTPRIVATE:__omp_offloading_[^"\\]*mapWithFirstprivate[^"\\]*]]\00"
@@ -42,9 +43,7 @@
 // INT128: @.omp_offloading.entry_name{{[0-9.]*}} = {{.*}} c"[[OFFLOAD_INT128_R1:__omp_offloading_[^"\\]*mapInt128[^"\\]*]]\00"
 
 // HOST: define {{.*}}mapWithPrivate
-// HOST: call {{.*}} @.[[OFFLOAD_PRIVATE]].region_id
-// HOST-NOT: offload_maptypes
-// HOST-SAME: {{$}}
+// HOST: call {{.*}} @.[[OFFLOAD_PRIVATE]].region_id{{.*}} @[[MAPTYPES_PRIVATE]]
 //
 // CHECK: define {{.*}} void @[[OFFLOAD_PRIVATE]]()
 // CHECK: call void ({{.*}}@[[OUTLINE_PRIVATE:.omp_outlined.[.0-9]*]]
Index: clang/test/OpenMP/target_map_codegen.cpp
===
--- clang/test/OpenMP/target_map_codegen.cpp
+++ clang/test/OpenMP/target_map_codegen.cpp
@@ -1307,12 +1307,26 @@
 
 #endif
 ///==///
-// RUN: %clang_cc1 -DCK19 -verify -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -emit-llvm %s -o - | FileCheck -allow-deprecated-dag-overlap  %s --check-prefix CK19 --check-prefix CK19-64
+// RUN: %clang_cc1 -DUSE -DCK19 -verify -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -emit-llvm %s -o - | FileCheck -allow-deprecated-dag-overlap  %s --check-prefixes=CK19,CK19-64,CK19-USE
+// RUN: %clang_cc1 -DUSE -DCK19 -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -std=c++11 -triple powerpc64le-unknown-unknown -emit-pch -o %t %s
+// RUN: %clang_cc1 -DUSE -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck -allow-deprecated-dag-overlap  %s  --check-prefixes=CK19,CK19-64,CK19-USE
+// RUN: %clang_cc1 -DUSE -DCK19 -verify -fopenmp -fopenmp-targets=i386-pc-linux-gnu -x c++ -triple i386-unknown-unknown -emit-llvm %s -o - | FileCheck -allow-deprecated-dag-overlap  %s  --check-prefixes=CK19,CK19-32,CK19-USE
+// RUN: %clang_cc1 -DUSE -DCK19 -fopenmp -fopenmp-targets=i386-pc-linux-gnu -x c++ -std=c++11 -triple i386-unknown-unknown -emit-pch -o %t %s
+// RUN: %clang_cc1 -DUSE -fopenmp -fopenmp-targets=i386-pc-linux-gnu -x c++ -triple i386-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck -allow-deprecated-dag-overlap  %s  --check-prefixes=CK19,CK19-32,CK19-USE
+
+// RUN: %clang_cc1 -DUSE -DCK19 -verify -fopenmp-simd -fopenmp-targets=powerpc64le-ibm-linux-gnu -x

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

2020-07-16 Thread Alexey Bataev via Phabricator via cfe-commits 
ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7948-7949
+   MapFlagsArrayTy ,
+   const llvm::DenseSet  =
+   llvm::DenseSet()) const {
 // We have to process the component lists that relate with the same

Use `llvm::DenseSet> `.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9439
 llvm::DenseMap LambdaPointers;
+llvm::DenseSet MappedVarSet;
 

`llvm::DenseSet> MappedVarSet;`



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9469
+if (!CI->capturesThis())
+  MappedVarSet.insert(CI->getCapturedVar()->getCanonicalDecl());
+else

No need to get canonical decl here for `llvm::DenseSet>`


Repository:
  rG LLVM Github Monorepo

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

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

2020-07-16 Thread Alexey Bataev via Phabricator via cfe-commits 
ABataev added a comment.

I would add checks for mapping of `declare target to/link` vars and checked if 
they work in runtime.




Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9468-9469
  CurSizes, CurMapTypes, PartialStruct);
+if (!CI->capturesThis())
+  MappedVarSet.insert(CI->getCapturedVar()->getCanonicalDecl());
+else

I would check that we capture a variable. We may capture VLA here as well.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9470-9471
+  MappedVarSet.insert(CI->getCapturedVar()->getCanonicalDecl());
+else
+  MappedVarSet.insert(nullptr);
 if (CurBasePointers.empty())

No need to insert `nullptr` here, it is not used later.


Repository:
  rG LLVM Github Monorepo

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

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

2020-07-15 Thread Joel E. Denny via Phabricator via cfe-commits 
jdenny created this revision.
jdenny added reviewers: ABataev, jdoerfert, hfinkel, Meinersbur, kkwli0, 
grokos, sfantao, gtbercea, Hahnfeld.
Herald added subscribers: cfe-commits, sstefan1, guansong, yaxunl.
Herald added a project: clang.

For example, without this patch:

  $ cat test.c
  int main() {
int x[3];
#pragma omp target map(tofrom:x[0:3])
  #ifdef USE 
x[0] = 1 
  #endif
;   
return 0;
  }
  $ clang -fopenmp -fopenmp-targets=nvptx64-nvida-cuda -S -emit-llvm test.c
  $ grep '^@.offload_maptypes' test.ll
  $ echo $?
  1
  $ clang -fopenmp -fopenmp-targets=nvptx64-nvida-cuda -S -emit-llvm test.c \
  -DUSE
  $ grep '^@.offload_maptypes' test.ll
  @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 35] 

With this patch, both greps produce the same result.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D83922

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/test/OpenMP/target_map_codegen.cpp
  clang/test/OpenMP/target_teams_map_codegen.cpp

Index: clang/test/OpenMP/target_teams_map_codegen.cpp
===
--- clang/test/OpenMP/target_teams_map_codegen.cpp
+++ clang/test/OpenMP/target_teams_map_codegen.cpp
@@ -20,15 +20,16 @@
 #ifndef HEADER
 #define HEADER
 
+// HOST: @[[MAPTYPES_PRIVATE:.offload_maptypes[0-9.]*]] = private {{.*}}constant [2 x i64] [i64 35, i64 35]
 // HOST: @[[MAPTYPES_FIRSTPRIVATE:.offload_maptypes[0-9.]*]] = private {{.*}}constant [2 x i64] [i64 35, i64 35]
 // HOST: @[[MAPTYPES_REDUCTION:.offload_maptypes[0-9.]*]] = private {{.*}}constant [2 x i64] [i64 35, i64 35]
 // HOST: @[[MAPTYPES_FROM:.offload_maptypes[0-9.]*]] = private {{.*}}constant [1 x i64] [i64 34]
 // HOST: @[[MAPTYPES_TO:.offload_maptypes[0-9.]*]] = private {{.*}}constant [1 x i64] [i64 33]
 // HOST: @[[MAPTYPES_ALLOC:.offload_maptypes[0-9.]*]] = private {{.*}}constant [1 x i64] [i64 32]
-// HOST: @[[MAPTYPES_ARRAY_R0:.offload_maptypes[0-9.]*]] = private {{.*}}constant [2 x i64] [i64 35, i64 35]
-// HOST: @[[MAPTYPES_ARRAY_R1:.offload_maptypes[0-9.]*]] = private {{.*}}constant [2 x i64] [i64 33, i64 33]
-// HOST-INT128: @[[MAPTYPES_INT128_R0:.offload_maptypes[0-9.]*]] = private {{.*}}constant [2 x i64] [i64 35, i64 35]
-// HOST-INT128: @[[MAPTYPES_INT128_R1:.offload_maptypes[0-9.]*]] = private {{.*}}constant [2 x i64] [i64 34, i64 34]
+// HOST: @[[MAPTYPES_ARRAY_R0:.offload_maptypes[0-9.]*]] = private {{.*}}constant [3 x i64] [i64 35, i64 35, i64 35]
+// HOST: @[[MAPTYPES_ARRAY_R1:.offload_maptypes[0-9.]*]] = private {{.*}}constant [3 x i64] [i64 33, i64 33, i64 33]
+// HOST-INT128: @[[MAPTYPES_INT128_R0:.offload_maptypes[0-9.]*]] = private {{.*}}constant [3 x i64] [i64 35, i64 35, i64 35]
+// HOST-INT128: @[[MAPTYPES_INT128_R1:.offload_maptypes[0-9.]*]] = private {{.*}}constant [3 x i64] [i64 34, i64 34, i64 34]
 //
 // CHECK: @.omp_offloading.entry_name{{[0-9.]*}} = {{.*}} c"[[OFFLOAD_PRIVATE:__omp_offloading_[^"\\]*mapWithPrivate[^"\\]*]]\00"
 // CHECK: @.omp_offloading.entry_name{{[0-9.]*}} = {{.*}} c"[[OFFLOAD_FIRSTPRIVATE:__omp_offloading_[^"\\]*mapWithFirstprivate[^"\\]*]]\00"
@@ -42,9 +43,7 @@
 // INT128: @.omp_offloading.entry_name{{[0-9.]*}} = {{.*}} c"[[OFFLOAD_INT128_R1:__omp_offloading_[^"\\]*mapInt128[^"\\]*]]\00"
 
 // HOST: define {{.*}}mapWithPrivate
-// HOST: call {{.*}} @.[[OFFLOAD_PRIVATE]].region_id
-// HOST-NOT: offload_maptypes
-// HOST-SAME: {{$}}
+// HOST: call {{.*}} @.[[OFFLOAD_PRIVATE]].region_id{{.*}} @[[MAPTYPES_PRIVATE]]
 //
 // CHECK: define {{.*}} void @[[OFFLOAD_PRIVATE]]()
 // CHECK: call void ({{.*}}@[[OUTLINE_PRIVATE:.omp_outlined.[.0-9]*]]
Index: clang/test/OpenMP/target_map_codegen.cpp
===
--- clang/test/OpenMP/target_map_codegen.cpp
+++ clang/test/OpenMP/target_map_codegen.cpp
@@ -1307,12 +1307,26 @@
 
 #endif
 ///==///
-// RUN: %clang_cc1 -DCK19 -verify -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -emit-llvm %s -o - | FileCheck -allow-deprecated-dag-overlap  %s --check-prefix CK19 --check-prefix CK19-64
+// RUN: %clang_cc1 -DUSE -DCK19 -verify -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -emit-llvm %s -o - | FileCheck -allow-deprecated-dag-overlap  %s --check-prefixes=CK19,CK19-64,CK19-USE
+// RUN: %clang_cc1 -DUSE -DCK19 -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -std=c++11 -triple powerpc64le-unknown-unknown -emit-pch -o %t %s
+// RUN: %clang_cc1 -DUSE -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck -allow-deprecated-dag-overlap  %s  --check-prefixes=CK19,CK19-64,CK19-USE
+// RUN: %clang_cc1 -DUSE -DCK19 -verify -fopenmp -fopenmp-targets=i386-pc-linux-gnu -x c++ -triple i386-unknown-unknown