[PATCH] D79677: [Clang][OpenMP][OMPBuilder] (1/4) Privatize `parallel` for `OMPBuilder`

2020-09-01 Thread Fady Ghanim via Phabricator via cfe-commits
fghanim added a comment.

In D79677#2248663 , @lebedev.ri wrote:

> Tests missing

I am not sure what to test here that isn't tested elsewhere in the series. This 
patch is the last in a series, and it represents the "usage" of the 
functionality added by the other patches. Each of the other patches has its 
test as a part of it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79677

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


[PATCH] D79677: [Clang][OpenMP][OMPBuilder] (1/4) Privatize `parallel` for `OMPBuilder`

2020-09-01 Thread Roman Lebedev via Phabricator via cfe-commits
lebedev.ri added a comment.

Tests missing


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79677

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


[PATCH] D79677: [Clang][OpenMP][OMPBuilder] (1/4) Privatize `parallel` for `OMPBuilder`

2020-08-31 Thread Fady Ghanim via Phabricator via cfe-commits
fghanim added a comment.

In D79677#2248197 , @kiranchandramohan 
wrote:

> What is the plan for this patch?

To commit it ... eventually :)
Once it (and the rest in the series) get reviewed. As it stands,  I cannot 
commit this patch without the rest in the series, and I cannot find reviewers 
for the other patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79677

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


[PATCH] D79677: [Clang][OpenMP][OMPBuilder] (1/4) Privatize `parallel` for `OMPBuilder`

2020-08-31 Thread Kiran Chandramohan via Phabricator via cfe-commits
kiranchandramohan added a comment.

What is the plan for this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79677

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


[PATCH] D79677: [Clang][OpenMP][OMPBuilder] (1/4) Privatize `parallel` for `OMPBuilder`

2020-07-01 Thread Fady Ghanim via Phabricator via cfe-commits
fghanim marked 3 inline comments as done.
fghanim added a comment.

In D79677#2119019 , @kiranchandramohan 
wrote:

> Is the ordering of code generation for clauses important?
>  copyin -> firstprivate -> barrier -> private


if we emitted a copyin, then prob. yes. otherwise no.




Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:1673
 //
 // TODO: This defaults to shared right now.
 auto PrivCB = [](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,

kiranchandramohan wrote:
> Should this change in this patch?
No, still used for shared variables.

but the todo should go away.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:1729-1736
+  if (!OMP_Entry->getTerminator()) {
+OMP_Entry->getInstList().push_back(EntryBI);
+  } else if (Builder.GetInsertBlock()->getTerminator()) {
+EntryBI->dropAllReferences();
+EntryBI->deleteValue();
+  } else {
+Builder.Insert(EntryBI);

kiranchandramohan wrote:
> Nit: What do these three cases correspond to? A comment might be useful.
Just checks to see what to do with the terminator of the entry block.

I'll add a comment in an update



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:1743
+  Builder.SetInsertPoint();
+  PrivateScope.ForceCleanup();
+  Builder.Insert(ContTI);

kiranchandramohan wrote:
> Not a comment about this patch: While the context makes it clear, the name 
> does not suggest that this function is emitting something.
you mean `ForceClean()`?
it forces the emission of cleanup code - instead of just as part of the 
Privatescope Dctor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79677



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


[PATCH] D79677: [Clang][OpenMP][OMPBuilder] (1/4) Privatize `parallel` for `OMPBuilder`

2020-06-29 Thread Kiran Chandramohan via Phabricator via cfe-commits
kiranchandramohan added a comment.

Is the ordering of code generation for clauses important?
copyin -> firstprivate -> barrier -> private




Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:1673
 //
 // TODO: This defaults to shared right now.
 auto PrivCB = [](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,

Should this change in this patch?



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:1729-1736
+  if (!OMP_Entry->getTerminator()) {
+OMP_Entry->getInstList().push_back(EntryBI);
+  } else if (Builder.GetInsertBlock()->getTerminator()) {
+EntryBI->dropAllReferences();
+EntryBI->deleteValue();
+  } else {
+Builder.Insert(EntryBI);

Nit: What do these three cases correspond to? A comment might be useful.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:1743
+  Builder.SetInsertPoint();
+  PrivateScope.ForceCleanup();
+  Builder.Insert(ContTI);

Not a comment about this patch: While the context makes it clear, the name does 
not suggest that this function is emitting something.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79677



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


[PATCH] D79677: [Clang][OpenMP][OMPBuilder] (1/4) Privatize `parallel` for `OMPBuilder`

2020-06-24 Thread Johannes Doerfert via Phabricator via cfe-commits
jdoerfert added a reviewer: kiranchandramohan.
jdoerfert added a comment.

@kiranchandramohan You can add yourself and others ;)

Can we test this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79677



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


[PATCH] D79677: [Clang][OpenMP][OMPBuilder] (1/4) Privatize `parallel` for `OMPBuilder`

2020-06-24 Thread Kiran Chandramohan via Phabricator via cfe-commits
kiranchandramohan added a comment.

Thanks @fghanim for this patch. I will get to this on Friday if you can add me 
as a reviewer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79677



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


[PATCH] D79677: [Clang][OpenMP][OMPBuilder] (1/4) Privatize `parallel` for `OMPBuilder`

2020-06-15 Thread Fady Ghanim via Phabricator via cfe-commits
fghanim added a comment.

ping - please suggest reviewers I can add to review the clang side of things?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79677



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