[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

> I applied D69853 , D69785 
> , D69922  
> to my local build and found that D69922  is 
> referring to OpenMPIRBuilder.h in llvm/Frontend whereas in D69785 
>  it was introduced in 
> llvm/Transforms/Utils/OpenMPIRBuilder.h

I merged the first few patches. There are some I'm still waiting for the green 
light but the location question is now settled.




Comment at: clang/include/clang/Driver/Options.td:1643-1644
   HelpText<"Emit OpenMP code only for SIMD-based constructs.">;
+def fopenmp_enable_irbuilder : Flag<["-"], "fopenmp-enable-irbuilder">, 
Group, Flags<[CC1Option, NoArgumentUnused, HelpHidden]>,
+  HelpText<"Use the experimental OpenMP-IR-Builder codegen path.">;
 def fno_openmp_simd : Flag<["-"], "fno-openmp-simd">, Group, 
Flags<[CC1Option, NoArgumentUnused]>;

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > Do we need to expose this option to driver or it is enough to have just a 
> > > frontend option? If still need to have a driver option, add a test for 
> > > driver.
> > How do I write a frontend option? Anything that we can query in the 
> > lib/CodeGen is fine with me. (I don't even need an option if we turn this 
> > on by default to get test coverage right away).
> You nedd to move to CC1Options.td file. It means that you can't use it 
> direcly when invoke the drive, instead you will need to use `-Xclang 
> -fopenmp-...`
I think people will need to use it in the short term, having it here seems 
therefor better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922



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

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb3c06db45611: [OpenMP] Use the OpenMP-IR-Builder (authored 
by jdoerfert).

Changed prior to commit:
  https://reviews.llvm.org/D69922?vs=233448=233458#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CMakeLists.txt
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/fopenmp.c
  clang/test/OpenMP/barrier_codegen.cpp
  clang/test/OpenMP/cancel_codegen.cpp

Index: clang/test/OpenMP/cancel_codegen.cpp
===
--- clang/test/OpenMP/cancel_codegen.cpp
+++ clang/test/OpenMP/cancel_codegen.cpp
@@ -2,6 +2,10 @@
 // RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -x c++ -std=c++11 -triple x86_64-apple-darwin13.4.0 -emit-pch -o %t %s
 // RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -std=c++11 -include-pch %t -fsyntax-only -verify %s -triple x86_64-apple-darwin13.4.0 -emit-llvm -o - | FileCheck %s
 
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-version=45 -fopenmp-enable-irbuilder -triple x86_64-apple-darwin13.4.0 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -fopenmp-enable-irbuilder -x c++ -std=c++11 -triple x86_64-apple-darwin13.4.0 -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -fopenmp-enable-irbuilder -std=c++11 -include-pch %t -fsyntax-only -verify %s -triple x86_64-apple-darwin13.4.0 -emit-llvm -o - | FileCheck %s
+
 // RUN: %clang_cc1 -verify -fopenmp-simd -fopenmp-version=45 -triple x86_64-apple-darwin13.4.0 -emit-llvm -o - %s | FileCheck --check-prefix SIMD-ONLY0 %s
 // RUN: %clang_cc1 -fopenmp-simd -fopenmp-version=45 -x c++ -std=c++11 -triple x86_64-apple-darwin13.4.0 -emit-pch -o %t %s
 // RUN: %clang_cc1 -fopenmp-simd -fopenmp-version=45 -std=c++11 -include-pch %t -fsyntax-only -verify %s -triple x86_64-apple-darwin13.4.0 -emit-llvm -o - | FileCheck --check-prefix SIMD-ONLY0 %s
Index: clang/test/OpenMP/barrier_codegen.cpp
===
--- clang/test/OpenMP/barrier_codegen.cpp
+++ clang/test/OpenMP/barrier_codegen.cpp
@@ -1,6 +1,14 @@
-// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,CLANGCG
 // RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-pch -o %t %s
-// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,CLANGCG
+
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER
+
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -mllvm -openmp-ir-builder-optimistic-attributes -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER_OPT
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -mllvm -openmp-ir-builder-optimistic-attributes -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER_OPT
 
 // RUN: %clang_cc1 -verify -fopenmp-simd -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck --check-prefix SIMD-ONLY0 %s
 // RUN: %clang_cc1 -fopenmp-simd -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-pch -o %t %s
@@ -34,6 +42,13 @@
   return tmain(argc) + tmain(argv[0][0]) + a;
 }
 
+// CLANGCG:declare i32 @__kmpc_global_thread_num(%struct.ident_t*)
+// CLANGCG-NOT:#
+// IRBUILDER:  ; Function Attrs: nounwind
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*) #
+// IRBUILDER_OPT:  ; Function Attrs: nofree nosync nounwind readonly
+// IRBUILDER_OPT-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*) #
+
 // CHECK: define {{.+}} [[TMAIN_INT]](
 // CHECK: 

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 233448.
jdoerfert marked 3 inline comments as done.
jdoerfert added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CMakeLists.txt
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/fopenmp.c
  clang/test/OpenMP/barrier_codegen.cpp
  clang/test/OpenMP/cancel_codegen.cpp

Index: clang/test/OpenMP/cancel_codegen.cpp
===
--- clang/test/OpenMP/cancel_codegen.cpp
+++ clang/test/OpenMP/cancel_codegen.cpp
@@ -2,6 +2,10 @@
 // RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -x c++ -std=c++11 -triple x86_64-apple-darwin13.4.0 -emit-pch -o %t %s
 // RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -std=c++11 -include-pch %t -fsyntax-only -verify %s -triple x86_64-apple-darwin13.4.0 -emit-llvm -o - | FileCheck %s
 
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-version=45 -fopenmp-enable-irbuilder -triple x86_64-apple-darwin13.4.0 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -fopenmp-enable-irbuilder -x c++ -std=c++11 -triple x86_64-apple-darwin13.4.0 -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -fopenmp-enable-irbuilder -std=c++11 -include-pch %t -fsyntax-only -verify %s -triple x86_64-apple-darwin13.4.0 -emit-llvm -o - | FileCheck %s
+
 // RUN: %clang_cc1 -verify -fopenmp-simd -fopenmp-version=45 -triple x86_64-apple-darwin13.4.0 -emit-llvm -o - %s | FileCheck --check-prefix SIMD-ONLY0 %s
 // RUN: %clang_cc1 -fopenmp-simd -fopenmp-version=45 -x c++ -std=c++11 -triple x86_64-apple-darwin13.4.0 -emit-pch -o %t %s
 // RUN: %clang_cc1 -fopenmp-simd -fopenmp-version=45 -std=c++11 -include-pch %t -fsyntax-only -verify %s -triple x86_64-apple-darwin13.4.0 -emit-llvm -o - | FileCheck --check-prefix SIMD-ONLY0 %s
Index: clang/test/OpenMP/barrier_codegen.cpp
===
--- clang/test/OpenMP/barrier_codegen.cpp
+++ clang/test/OpenMP/barrier_codegen.cpp
@@ -1,6 +1,14 @@
-// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,CLANGCG
 // RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-pch -o %t %s
-// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,CLANGCG
+
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER
+
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -mllvm -openmp-ir-builder-optimistic-attributes -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER_OPT
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -mllvm -openmp-ir-builder-optimistic-attributes -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER_OPT
 
 // RUN: %clang_cc1 -verify -fopenmp-simd -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck --check-prefix SIMD-ONLY0 %s
 // RUN: %clang_cc1 -fopenmp-simd -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-pch -o %t %s
@@ -34,6 +42,13 @@
   return tmain(argc) + tmain(argv[0][0]) + a;
 }
 
+// CLANGCG:declare i32 @__kmpc_global_thread_num(%struct.ident_t*)
+// CLANGCG-NOT:#
+// IRBUILDER:  ; Function Attrs: nounwind
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*) #
+// IRBUILDER_OPT:  ; Function Attrs: nofree nosync nounwind readonly
+// IRBUILDER_OPT-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*) #
+
 // CHECK: define {{.+}} [[TMAIN_INT]](
 // CHECK: [[GTID:%.+]] = call i32 @__kmpc_global_thread_num([[IDENT_T]]* [[LOC]])
 // CHECK: call void @__kmpc_barrier([[IDENT_T]]* 

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 3 inline comments as done.
jdoerfert added a comment.

In D69922#1742392 , @ABataev wrote:

> In D69922#1741659 , @jdoerfert wrote:
>
> > Use IRBuilder for cancel barriers
>
>
> In general, it would better to implement cancellation barriers in the 
> separate patch.  Same for OpenMPIrBuilder.


Why would you think that splitting these closely related concepts would be 
beneficial?

In D69922#1751827 , @kiranchandramohan 
wrote:

> Thanks @jdoerfert for working on this.
>
> Sorry for being late to the party. I am trying to implement one trivial 
> directive (Flush) and one slightly more involved (not decided).
>
> I applied D69853 , D69785 
> , D69922  
> to my local build and found that D69922  is 
> referring to OpenMPIRBuilder.h in llvm/Frontend whereas in D69785 
>  it was introduced in 
> llvm/Transforms/Utils/OpenMPIRBuilder.h


The conversation (on the llvm-dev list) seems to have settled on 
`{include/llvm,lib}/frontend/OpenMP/`. I will move the files around once we 
finally settle these reviews that haven't moved (more than 2 comments at a 
time).
As you can see, I have a huge backlog here and that is really hard to modify. 
You have a problem now as you depend on these. @hfinkel mentioned it in his 
email to the llvm-dev (about review etiquette), we need to
minimize iterations in reviews and actually review patches not only a few lines 
every time.




Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3492
+  CGF.createBasicBlock(".cancel.exit", IP.getBlock()->getParent());
+  OMPBuilder->setCancellationBlock(ExitBB);
+  CGF.Builder.SetInsertPoint(ExitBB);

ABataev wrote:
> Maybe, instead of saving the state, pass the pointer to the cancel block as a 
> parameter to the `CreateBarrier` function?
All of this goes away in D70258, as it should. The frontend when creating a 
barrier does not need to know if it is cancelable and if so, where the cancel 
block is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922



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

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

Thanks @jdoerfert for working on this.

Sorry for being late to the party. I am trying to implement one trivial 
directive (Flush) and one slightly more involved (not decided).

I applied D69853 , D69785 
, D69922  to 
my local build and found that D69922  is 
referring to OpenMPIRBuilder.h in llvm/Frontend whereas in D69785 
 it was introduced in 
llvm/Transforms/Utils/OpenMPIRBuilder.h




Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:29
 #include "llvm/Support/raw_ostream.h"
+#include "llvm/Frontend/OpenMPIRBuilder.h"
 #include 

D69785 has this file in llvm/Transforms/Utils/OpenMPIRBuilder.h



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:55
 #include "llvm/IR/Module.h"
+#include "llvm/Frontend/OpenMPIRBuilder.h"
 #include "llvm/IR/ProfileSummary.h"

D69785 has this file in llvm/Transforms/Utils/OpenMPIRBuilder.h


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922



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

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3489
+OMPRegionInfo->hasCancel()) {
+  auto IP = CGF.Builder.saveIP();
+  llvm::BasicBlock *ExitBB =

Also, you need to introduce/use RAII that saves/restores insertion point 
automatically.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3492
+  CGF.createBasicBlock(".cancel.exit", IP.getBlock()->getParent());
+  OMPBuilder->setCancellationBlock(ExitBB);
+  CGF.Builder.SetInsertPoint(ExitBB);

Maybe, instead of saving the state, pass the pointer to the cancel block as a 
parameter to the `CreateBarrier` function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922



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

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 229233.
jdoerfert added a comment.

Adjust paths and test case options


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CMakeLists.txt
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/fopenmp.c
  clang/test/OpenMP/barrier_codegen.cpp
  clang/test/OpenMP/cancel_codegen.cpp

Index: clang/test/OpenMP/cancel_codegen.cpp
===
--- clang/test/OpenMP/cancel_codegen.cpp
+++ clang/test/OpenMP/cancel_codegen.cpp
@@ -2,6 +2,10 @@
 // RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -x c++ -std=c++11 -triple x86_64-apple-darwin13.4.0 -emit-pch -o %t %s
 // RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -std=c++11 -include-pch %t -fsyntax-only -verify %s -triple x86_64-apple-darwin13.4.0 -emit-llvm -o - | FileCheck %s
 
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-version=45 -fopenmp-enable-irbuilder -triple x86_64-apple-darwin13.4.0 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -fopenmp-enable-irbuilder -x c++ -std=c++11 -triple x86_64-apple-darwin13.4.0 -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -fopenmp-enable-irbuilder -std=c++11 -include-pch %t -fsyntax-only -verify %s -triple x86_64-apple-darwin13.4.0 -emit-llvm -o - | FileCheck %s
+
 // RUN: %clang_cc1 -verify -fopenmp-simd -fopenmp-version=45 -triple x86_64-apple-darwin13.4.0 -emit-llvm -o - %s | FileCheck --check-prefix SIMD-ONLY0 %s
 // RUN: %clang_cc1 -fopenmp-simd -fopenmp-version=45 -x c++ -std=c++11 -triple x86_64-apple-darwin13.4.0 -emit-pch -o %t %s
 // RUN: %clang_cc1 -fopenmp-simd -fopenmp-version=45 -std=c++11 -include-pch %t -fsyntax-only -verify %s -triple x86_64-apple-darwin13.4.0 -emit-llvm -o - | FileCheck --check-prefix SIMD-ONLY0 %s
Index: clang/test/OpenMP/barrier_codegen.cpp
===
--- clang/test/OpenMP/barrier_codegen.cpp
+++ clang/test/OpenMP/barrier_codegen.cpp
@@ -1,6 +1,14 @@
-// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,CLANGCG
 // RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-pch -o %t %s
-// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,CLANGCG
+
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER
+
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -mllvm -openmp-ir-builder-optimistic-attributes -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER_OPT
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -mllvm -openmp-ir-builder-optimistic-attributes -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER_OPT
 
 // RUN: %clang_cc1 -verify -fopenmp-simd -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck --check-prefix SIMD-ONLY0 %s
 // RUN: %clang_cc1 -fopenmp-simd -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-pch -o %t %s
@@ -34,6 +42,13 @@
   return tmain(argc) + tmain(argv[0][0]) + a;
 }
 
+// CLANGCG:declare i32 @__kmpc_global_thread_num(%struct.ident_t*)
+// CLANGCG-NOT:#
+// IRBUILDER:  ; Function Attrs: nounwind
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*) #
+// IRBUILDER_OPT:  ; Function Attrs: nofree nosync nounwind readonly
+// IRBUILDER_OPT-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*) #
+
 // CHECK: define {{.+}} [[TMAIN_INT]](
 // CHECK: [[GTID:%.+]] = call i32 @__kmpc_global_thread_num([[IDENT_T]]* [[LOC]])
 // CHECK: call void @__kmpc_barrier([[IDENT_T]]* 

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D69922#1741659 , @jdoerfert wrote:

> Use IRBuilder for cancel barriers


In general, it would better to implement cancellation barriers in the separate 
patch.  Same for OpenMPIrBuilder.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922



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

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 228801.
jdoerfert added a comment.

Use IRBuilder for cancel barriers


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CMakeLists.txt
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/fopenmp.c
  clang/test/OpenMP/barrier_codegen.cpp
  clang/test/OpenMP/cancel_codegen.cpp
  llvm/lib/Frontend/OpenMPIRBuilder.cpp

Index: llvm/lib/Frontend/OpenMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMPIRBuilder.cpp
@@ -221,7 +221,6 @@
 break;
   }
 
-  // Set new insertion point for the internal builder.
   Constant *SrcLocStr = getOrCreateSrcLocStr(Loc);
   Value *Args[] = {getOrCreateIdent(SrcLocStr, BarrierLocFlags),
getOrCreateThreadID(getOrCreateIdent(SrcLocStr))};
@@ -239,6 +238,12 @@
 Builder.CreateCondBr(Cmp, ContinuationBB, CancellationBlock,
  /* TODO weight */ nullptr, nullptr);
 Builder.SetInsertPoint(ContinuationBB);
+assert(CancellationBlock->getParent() == BB->getParent() &&
+   "Unexpected cancellation block parent!");
+
+// TODO: This is a workaround for now, we always reset the cancellation
+// block until we manage it ourselves here.
+CancellationBlock = nullptr;
   }
 
   return Builder.saveIP();
Index: clang/test/OpenMP/cancel_codegen.cpp
===
--- clang/test/OpenMP/cancel_codegen.cpp
+++ clang/test/OpenMP/cancel_codegen.cpp
@@ -2,6 +2,10 @@
 // RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -x c++ -std=c++11 -triple x86_64-apple-darwin13.4.0 -emit-pch -o %t %s
 // RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -std=c++11 -include-pch %t -fsyntax-only -verify %s -triple x86_64-apple-darwin13.4.0 -emit-llvm -o - | FileCheck %s
 
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-version=45 -fopenmp-enable-irbuilder -triple x86_64-apple-darwin13.4.0 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -x c++ -std=c++11 -triple x86_64-apple-darwin13.4.0 -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -fopenmp-enable-irbuilder -std=c++11 -include-pch %t -fsyntax-only -verify %s -triple x86_64-apple-darwin13.4.0 -emit-llvm -o - | FileCheck %s
+
 // RUN: %clang_cc1 -verify -fopenmp-simd -fopenmp-version=45 -triple x86_64-apple-darwin13.4.0 -emit-llvm -o - %s | FileCheck --check-prefix SIMD-ONLY0 %s
 // RUN: %clang_cc1 -fopenmp-simd -fopenmp-version=45 -x c++ -std=c++11 -triple x86_64-apple-darwin13.4.0 -emit-pch -o %t %s
 // RUN: %clang_cc1 -fopenmp-simd -fopenmp-version=45 -std=c++11 -include-pch %t -fsyntax-only -verify %s -triple x86_64-apple-darwin13.4.0 -emit-llvm -o - | FileCheck --check-prefix SIMD-ONLY0 %s
Index: clang/test/OpenMP/barrier_codegen.cpp
===
--- clang/test/OpenMP/barrier_codegen.cpp
+++ clang/test/OpenMP/barrier_codegen.cpp
@@ -1,6 +1,14 @@
-// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,CLANGCG
 // RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-pch -o %t %s
-// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,CLANGCG
+
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER
+
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -mllvm -openmp-ir-builder-optimistic-attributes -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER_OPT
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -mllvm -openmp-ir-builder-optimistic-attributes 

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922



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

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 228772.
jdoerfert added a comment.

update test case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/fopenmp.c
  clang/test/OpenMP/barrier_codegen.cpp

Index: clang/test/OpenMP/barrier_codegen.cpp
===
--- clang/test/OpenMP/barrier_codegen.cpp
+++ clang/test/OpenMP/barrier_codegen.cpp
@@ -1,6 +1,10 @@
-// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,CLANGCG
 // RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-pch -o %t %s
-// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,CLANGCG
+
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER
 
 // RUN: %clang_cc1 -verify -fopenmp-simd -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck --check-prefix SIMD-ONLY0 %s
 // RUN: %clang_cc1 -fopenmp-simd -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-pch -o %t %s
@@ -14,6 +18,10 @@
 // CHECK-DAG: [[EXPLICIT_BARRIER_LOC:@.+]] = {{.+}} [[IDENT_T]] { i32 0, i32 34, i32 0, i32 0, i8* getelementptr inbounds ([{{[0-9]+}} x i8], [{{[0-9]+}} x i8]* @{{.+}}, i32 0, i32 0) }
 // CHECK-DAG: [[LOC:@.+]] = {{.+}} [[IDENT_T]] { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([{{[0-9]+}} x i8], [{{[0-9]+}} x i8]* @{{.+}}, i32 0, i32 0) }
 
+// CLANGCG-NOT: nounwind
+// IRBUILDER:  ; Function Attrs: nounwind
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*)
+
 void foo() {}
 
 template 
Index: clang/test/Driver/fopenmp.c
===
--- clang/test/Driver/fopenmp.c
+++ clang/test/Driver/fopenmp.c
@@ -124,6 +124,12 @@
 // CHECK-LD-STATIC-IOMP5-NO-BDYNAMIC: "-{{B?}}static" {{.*}} "-liomp5"
 // CHECK-LD-STATIC-IOMP5-NO-BDYNAMIC-NOT: "-Bdynamic"
 //
+// RUN: %clang -target x86_64-linux-gnu -fopenmp -fopenmp-enable-irbuilder -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CC1-OPENMPIRBUILDER
+//
+// CHECK-CC1-OPENMPIRBUILDER: "-cc1"
+// CHECK-CC1-OPENMPIRBUILDER-SAME: "-fopenmp"
+// CHECK-CC1-OPENMPIRBUILDER-SAME: "-fopenmp-enable-irbuilder"
+//
 // We'd like to check that the default is sane, but until we have the ability
 // to *always* semantically analyze OpenMP without always generating runtime
 // calls (in the event of an unsupported runtime), we don't have a good way to
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2996,6 +2996,8 @@
   Opts.OpenMP && !Args.hasArg(options::OPT_fnoopenmp_use_tls);
   Opts.OpenMPIsDevice =
   Opts.OpenMP && Args.hasArg(options::OPT_fopenmp_is_device);
+  Opts.OpenMPIRBuilder =
+  Opts.OpenMP && Args.hasArg(options::OPT_fopenmp_enable_irbuilder);
   bool IsTargetSpecified =
   Opts.OpenMPIsDevice || Args.hasArg(options::OPT_fopenmp_targets_EQ);
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4555,6 +4555,7 @@
 CmdArgs.push_back("-fnoopenmp-use-tls");
   Args.AddLastArg(CmdArgs, options::OPT_fopenmp_simd,
   options::OPT_fno_openmp_simd);
+  Args.AddAllArgs(CmdArgs, options::OPT_fopenmp_enable_irbuilder);
   Args.AddAllArgs(CmdArgs, options::OPT_fopenmp_version_EQ);
   Args.AddAllArgs(CmdArgs, options::OPT_fopenmp_cuda_number_of_sm_EQ);
   Args.AddAllArgs(CmdArgs, options::OPT_fopenmp_cuda_blocks_per_sm_EQ);
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- 

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: clang/test/OpenMP/barrier_codegen.cpp:22
+// CLANGCG-NOT: readonly
+// IRBUILDER:  ; Function Attrs: nofree nosync nounwind readonly
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*)

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > ABataev wrote:
> > > > > > > jdoerfert wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > Not sure about correct use of `nosync` and `readonly` 
> > > > > > > > > attributes. OpenMP runtime uses lazy initialization of the 
> > > > > > > > > runtime library and when any runtime function is called, the 
> > > > > > > > > inner parts of the OpenMP runtime are initialized 
> > > > > > > > > automatically. It may use some sync primitives and may modify 
> > > > > > > > > memory, I assume. Same about `nofree`.
> > > > > > > > There are two versions of these functions, host and device. I 
> > > > > > > > assume host functions are not inlined but device functions 
> > > > > > > > might be. This is basically all the modes we support right now.
> > > > > > > > 
> > > > > > > > If we do not inline the function (host) we don't necessarily 
> > > > > > > > care what they do but what effect the user can expect.
> > > > > > > > The user can not expect to synchronize through 
> > > > > > > > `__kmpc_global_thread_num` calls in a defined way, thus 
> > > > > > > > `nosync`.
> > > > > > > > Similarly, from the users perspective there is no way to 
> > > > > > > > determine if something was written or freed, no matter how many 
> > > > > > > > of these calls I issue and under which control conditions, all 
> > > > > > > > I see is the number as a result. Thus, `readonly` and `nofree`. 
> > > > > > > > I believe `readnone` is even fine here but it might not work 
> > > > > > > > for the device version (see below) so I removed it.
> > > > > > > > 
> > > > > > > > If we do inline the function (device) we need to make sure the 
> > > > > > > > attributes are compatible with the inlined code to not cause 
> > > > > > > > UB. The code of `__kmpc_global_thread_num` at least does not 
> > > > > > > > write anything and only reads stuff (as far as I can tell).
> > > > > > > > 
> > > > > > > > Please correct me if I overlooked something. 
> > > > > > > But someone may try to inline the host-based runtime or try to 
> > > > > > > use LTO with it.
> > > > > > > The question is not about the user expectations but about the 
> > > > > > > optimizations which can be triggered with these attributes.
> > > > > > This is our runtime and we have supported and unsupported usage 
> > > > > > models.
> > > > > Hmm, I don't think this the right approach. Plus, you still did not 
> > > > > answer about optimizations. Maybe, currently, these attributes won't 
> > > > > trigger dangerous optimizations but they can do this in the future 
> > > > > and it may lead to unpredictable results. I would use the pessimistic 
> > > > > model here rather than over-optimistic.
> > > > I did (try to) describe why there cannot be any problems wrt. 
> > > > optimizations:
> > > > The specified behavior of the runtime call is _as if_ it is `readonly`, 
> > > > `nofree`, and `nosync`.
> > > > That is, from the perspective of the compiler this is true and 
> > > > optimizations are allowed to use that fact.
> > > >  
> > > > The fact that the first ever runtime call initializes the runtime is 
> > > > neither part of the specification nor of the observable behavior. If we 
> > > > change the order between two call to `__kmpc_global_thread_num`, or 
> > > > similar calls, we cannot observe if/which one initialized the runtime 
> > > > and which read only stuff.
> > > Here is the code of this function from the libomp:
> > > ```
> > >   int gtid;
> > > 
> > >   if (!__kmp_init_serial) {
> > > gtid = KMP_GTID_DNE;
> > >   } else
> > > #ifdef KMP_TDATA_GTID
> > >   if (TCR_4(__kmp_gtid_mode) >= 3) {
> > > KA_TRACE(1000, ("*** __kmp_get_global_thread_id_reg: using TDATA\n"));
> > > gtid = __kmp_gtid;
> > >   } else
> > > #endif
> > >   if (TCR_4(__kmp_gtid_mode) >= 2) {
> > > KA_TRACE(1000, ("*** __kmp_get_global_thread_id_reg: using keyed 
> > > TLS\n"));
> > > gtid = __kmp_gtid_get_specific();
> > >   } else {
> > > KA_TRACE(1000,
> > >  ("*** __kmp_get_global_thread_id_reg: using internal 
> > > alg.\n"));
> > > gtid = __kmp_get_global_thread_id();
> > >   }
> > > 
> > >   /* we must be a new uber master sibling thread */
> > >   if (gtid == KMP_GTID_DNE) {
> > > KA_TRACE(10,
> > >  ("__kmp_get_global_thread_id_reg: Encountered new root 
> > > thread. "
> > >   "Registering a new gtid.\n"));
> > > __kmp_acquire_bootstrap_lock(&__kmp_initz_lock);
> > > if (!__kmp_init_serial) {
> > >   __kmp_do_serial_initialize();
> > >   gtid = 

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/barrier_codegen.cpp:22
+// CLANGCG-NOT: readonly
+// IRBUILDER:  ; Function Attrs: nofree nosync nounwind readonly
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*)

jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > jdoerfert wrote:
> > > > > ABataev wrote:
> > > > > > jdoerfert wrote:
> > > > > > > ABataev wrote:
> > > > > > > > Not sure about correct use of `nosync` and `readonly` 
> > > > > > > > attributes. OpenMP runtime uses lazy initialization of the 
> > > > > > > > runtime library and when any runtime function is called, the 
> > > > > > > > inner parts of the OpenMP runtime are initialized 
> > > > > > > > automatically. It may use some sync primitives and may modify 
> > > > > > > > memory, I assume. Same about `nofree`.
> > > > > > > There are two versions of these functions, host and device. I 
> > > > > > > assume host functions are not inlined but device functions might 
> > > > > > > be. This is basically all the modes we support right now.
> > > > > > > 
> > > > > > > If we do not inline the function (host) we don't necessarily care 
> > > > > > > what they do but what effect the user can expect.
> > > > > > > The user can not expect to synchronize through 
> > > > > > > `__kmpc_global_thread_num` calls in a defined way, thus `nosync`.
> > > > > > > Similarly, from the users perspective there is no way to 
> > > > > > > determine if something was written or freed, no matter how many 
> > > > > > > of these calls I issue and under which control conditions, all I 
> > > > > > > see is the number as a result. Thus, `readonly` and `nofree`. I 
> > > > > > > believe `readnone` is even fine here but it might not work for 
> > > > > > > the device version (see below) so I removed it.
> > > > > > > 
> > > > > > > If we do inline the function (device) we need to make sure the 
> > > > > > > attributes are compatible with the inlined code to not cause UB. 
> > > > > > > The code of `__kmpc_global_thread_num` at least does not write 
> > > > > > > anything and only reads stuff (as far as I can tell).
> > > > > > > 
> > > > > > > Please correct me if I overlooked something. 
> > > > > > But someone may try to inline the host-based runtime or try to use 
> > > > > > LTO with it.
> > > > > > The question is not about the user expectations but about the 
> > > > > > optimizations which can be triggered with these attributes.
> > > > > This is our runtime and we have supported and unsupported usage 
> > > > > models.
> > > > Hmm, I don't think this the right approach. Plus, you still did not 
> > > > answer about optimizations. Maybe, currently, these attributes won't 
> > > > trigger dangerous optimizations but they can do this in the future and 
> > > > it may lead to unpredictable results. I would use the pessimistic model 
> > > > here rather than over-optimistic.
> > > I did (try to) describe why there cannot be any problems wrt. 
> > > optimizations:
> > > The specified behavior of the runtime call is _as if_ it is `readonly`, 
> > > `nofree`, and `nosync`.
> > > That is, from the perspective of the compiler this is true and 
> > > optimizations are allowed to use that fact.
> > >  
> > > The fact that the first ever runtime call initializes the runtime is 
> > > neither part of the specification nor of the observable behavior. If we 
> > > change the order between two call to `__kmpc_global_thread_num`, or 
> > > similar calls, we cannot observe if/which one initialized the runtime and 
> > > which read only stuff.
> > Here is the code of this function from the libomp:
> > ```
> >   int gtid;
> > 
> >   if (!__kmp_init_serial) {
> > gtid = KMP_GTID_DNE;
> >   } else
> > #ifdef KMP_TDATA_GTID
> >   if (TCR_4(__kmp_gtid_mode) >= 3) {
> > KA_TRACE(1000, ("*** __kmp_get_global_thread_id_reg: using TDATA\n"));
> > gtid = __kmp_gtid;
> >   } else
> > #endif
> >   if (TCR_4(__kmp_gtid_mode) >= 2) {
> > KA_TRACE(1000, ("*** __kmp_get_global_thread_id_reg: using keyed 
> > TLS\n"));
> > gtid = __kmp_gtid_get_specific();
> >   } else {
> > KA_TRACE(1000,
> >  ("*** __kmp_get_global_thread_id_reg: using internal alg.\n"));
> > gtid = __kmp_get_global_thread_id();
> >   }
> > 
> >   /* we must be a new uber master sibling thread */
> >   if (gtid == KMP_GTID_DNE) {
> > KA_TRACE(10,
> >  ("__kmp_get_global_thread_id_reg: Encountered new root thread. 
> > "
> >   "Registering a new gtid.\n"));
> > __kmp_acquire_bootstrap_lock(&__kmp_initz_lock);
> > if (!__kmp_init_serial) {
> >   __kmp_do_serial_initialize();
> >   gtid = __kmp_gtid_get_specific();
> > } else {
> >   gtid = __kmp_register_root(FALSE);
> > }
> > __kmp_release_bootstrap_lock(&__kmp_initz_lock);
> > /*__kmp_printf( "+++ %d\n", gtid ); */ /* GROO */
> >   }
> > 
> >   

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.



Comment at: clang/test/OpenMP/barrier_codegen.cpp:22
+// CLANGCG-NOT: readonly
+// IRBUILDER:  ; Function Attrs: nofree nosync nounwind readonly
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*)

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > ABataev wrote:
> > > > > > > Not sure about correct use of `nosync` and `readonly` attributes. 
> > > > > > > OpenMP runtime uses lazy initialization of the runtime library 
> > > > > > > and when any runtime function is called, the inner parts of the 
> > > > > > > OpenMP runtime are initialized automatically. It may use some 
> > > > > > > sync primitives and may modify memory, I assume. Same about 
> > > > > > > `nofree`.
> > > > > > There are two versions of these functions, host and device. I 
> > > > > > assume host functions are not inlined but device functions might 
> > > > > > be. This is basically all the modes we support right now.
> > > > > > 
> > > > > > If we do not inline the function (host) we don't necessarily care 
> > > > > > what they do but what effect the user can expect.
> > > > > > The user can not expect to synchronize through 
> > > > > > `__kmpc_global_thread_num` calls in a defined way, thus `nosync`.
> > > > > > Similarly, from the users perspective there is no way to determine 
> > > > > > if something was written or freed, no matter how many of these 
> > > > > > calls I issue and under which control conditions, all I see is the 
> > > > > > number as a result. Thus, `readonly` and `nofree`. I believe 
> > > > > > `readnone` is even fine here but it might not work for the device 
> > > > > > version (see below) so I removed it.
> > > > > > 
> > > > > > If we do inline the function (device) we need to make sure the 
> > > > > > attributes are compatible with the inlined code to not cause UB. 
> > > > > > The code of `__kmpc_global_thread_num` at least does not write 
> > > > > > anything and only reads stuff (as far as I can tell).
> > > > > > 
> > > > > > Please correct me if I overlooked something. 
> > > > > But someone may try to inline the host-based runtime or try to use 
> > > > > LTO with it.
> > > > > The question is not about the user expectations but about the 
> > > > > optimizations which can be triggered with these attributes.
> > > > This is our runtime and we have supported and unsupported usage models.
> > > Hmm, I don't think this the right approach. Plus, you still did not 
> > > answer about optimizations. Maybe, currently, these attributes won't 
> > > trigger dangerous optimizations but they can do this in the future and it 
> > > may lead to unpredictable results. I would use the pessimistic model here 
> > > rather than over-optimistic.
> > I did (try to) describe why there cannot be any problems wrt. optimizations:
> > The specified behavior of the runtime call is _as if_ it is `readonly`, 
> > `nofree`, and `nosync`.
> > That is, from the perspective of the compiler this is true and 
> > optimizations are allowed to use that fact.
> >  
> > The fact that the first ever runtime call initializes the runtime is 
> > neither part of the specification nor of the observable behavior. If we 
> > change the order between two call to `__kmpc_global_thread_num`, or similar 
> > calls, we cannot observe if/which one initialized the runtime and which 
> > read only stuff.
> Here is the code of this function from the libomp:
> ```
>   int gtid;
> 
>   if (!__kmp_init_serial) {
> gtid = KMP_GTID_DNE;
>   } else
> #ifdef KMP_TDATA_GTID
>   if (TCR_4(__kmp_gtid_mode) >= 3) {
> KA_TRACE(1000, ("*** __kmp_get_global_thread_id_reg: using TDATA\n"));
> gtid = __kmp_gtid;
>   } else
> #endif
>   if (TCR_4(__kmp_gtid_mode) >= 2) {
> KA_TRACE(1000, ("*** __kmp_get_global_thread_id_reg: using keyed TLS\n"));
> gtid = __kmp_gtid_get_specific();
>   } else {
> KA_TRACE(1000,
>  ("*** __kmp_get_global_thread_id_reg: using internal alg.\n"));
> gtid = __kmp_get_global_thread_id();
>   }
> 
>   /* we must be a new uber master sibling thread */
>   if (gtid == KMP_GTID_DNE) {
> KA_TRACE(10,
>  ("__kmp_get_global_thread_id_reg: Encountered new root thread. "
>   "Registering a new gtid.\n"));
> __kmp_acquire_bootstrap_lock(&__kmp_initz_lock);
> if (!__kmp_init_serial) {
>   __kmp_do_serial_initialize();
>   gtid = __kmp_gtid_get_specific();
> } else {
>   gtid = __kmp_register_root(FALSE);
> }
> __kmp_release_bootstrap_lock(&__kmp_initz_lock);
> /*__kmp_printf( "+++ %d\n", gtid ); */ /* GROO */
>   }
> 
>   KMP_DEBUG_ASSERT(gtid >= 0);
> 
>   return gtid;
> 
> ```
> 
> As you can see, it is very complex. It has locks, in the debug mode it writes 
> something, etc. It is not only about the 

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/barrier_codegen.cpp:22
+// CLANGCG-NOT: readonly
+// IRBUILDER:  ; Function Attrs: nofree nosync nounwind readonly
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*)

jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > jdoerfert wrote:
> > > > > ABataev wrote:
> > > > > > Not sure about correct use of `nosync` and `readonly` attributes. 
> > > > > > OpenMP runtime uses lazy initialization of the runtime library and 
> > > > > > when any runtime function is called, the inner parts of the OpenMP 
> > > > > > runtime are initialized automatically. It may use some sync 
> > > > > > primitives and may modify memory, I assume. Same about `nofree`.
> > > > > There are two versions of these functions, host and device. I assume 
> > > > > host functions are not inlined but device functions might be. This is 
> > > > > basically all the modes we support right now.
> > > > > 
> > > > > If we do not inline the function (host) we don't necessarily care 
> > > > > what they do but what effect the user can expect.
> > > > > The user can not expect to synchronize through 
> > > > > `__kmpc_global_thread_num` calls in a defined way, thus `nosync`.
> > > > > Similarly, from the users perspective there is no way to determine if 
> > > > > something was written or freed, no matter how many of these calls I 
> > > > > issue and under which control conditions, all I see is the number as 
> > > > > a result. Thus, `readonly` and `nofree`. I believe `readnone` is even 
> > > > > fine here but it might not work for the device version (see below) so 
> > > > > I removed it.
> > > > > 
> > > > > If we do inline the function (device) we need to make sure the 
> > > > > attributes are compatible with the inlined code to not cause UB. The 
> > > > > code of `__kmpc_global_thread_num` at least does not write anything 
> > > > > and only reads stuff (as far as I can tell).
> > > > > 
> > > > > Please correct me if I overlooked something. 
> > > > But someone may try to inline the host-based runtime or try to use LTO 
> > > > with it.
> > > > The question is not about the user expectations but about the 
> > > > optimizations which can be triggered with these attributes.
> > > This is our runtime and we have supported and unsupported usage models.
> > Hmm, I don't think this the right approach. Plus, you still did not answer 
> > about optimizations. Maybe, currently, these attributes won't trigger 
> > dangerous optimizations but they can do this in the future and it may lead 
> > to unpredictable results. I would use the pessimistic model here rather 
> > than over-optimistic.
> I did (try to) describe why there cannot be any problems wrt. optimizations:
> The specified behavior of the runtime call is _as if_ it is `readonly`, 
> `nofree`, and `nosync`.
> That is, from the perspective of the compiler this is true and optimizations 
> are allowed to use that fact.
>  
> The fact that the first ever runtime call initializes the runtime is neither 
> part of the specification nor of the observable behavior. If we change the 
> order between two call to `__kmpc_global_thread_num`, or similar calls, we 
> cannot observe if/which one initialized the runtime and which read only stuff.
Here is the code of this function from the libomp:
```
  int gtid;

  if (!__kmp_init_serial) {
gtid = KMP_GTID_DNE;
  } else
#ifdef KMP_TDATA_GTID
  if (TCR_4(__kmp_gtid_mode) >= 3) {
KA_TRACE(1000, ("*** __kmp_get_global_thread_id_reg: using TDATA\n"));
gtid = __kmp_gtid;
  } else
#endif
  if (TCR_4(__kmp_gtid_mode) >= 2) {
KA_TRACE(1000, ("*** __kmp_get_global_thread_id_reg: using keyed TLS\n"));
gtid = __kmp_gtid_get_specific();
  } else {
KA_TRACE(1000,
 ("*** __kmp_get_global_thread_id_reg: using internal alg.\n"));
gtid = __kmp_get_global_thread_id();
  }

  /* we must be a new uber master sibling thread */
  if (gtid == KMP_GTID_DNE) {
KA_TRACE(10,
 ("__kmp_get_global_thread_id_reg: Encountered new root thread. "
  "Registering a new gtid.\n"));
__kmp_acquire_bootstrap_lock(&__kmp_initz_lock);
if (!__kmp_init_serial) {
  __kmp_do_serial_initialize();
  gtid = __kmp_gtid_get_specific();
} else {
  gtid = __kmp_register_root(FALSE);
}
__kmp_release_bootstrap_lock(&__kmp_initz_lock);
/*__kmp_printf( "+++ %d\n", gtid ); */ /* GROO */
  }

  KMP_DEBUG_ASSERT(gtid >= 0);

  return gtid;

```

As you can see, it is very complex. It has locks, in the debug mode it writes 
something, etc. It is not only about the initialization, there are some other 
calls (like `__kmp_get_global_thread_id()`, which really writes something). Do 
you really think it is safe to mark this as readonly and nosync?


Repository:
  rG LLVM Github Monorepo

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

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added a comment.
jdoerfert added inline comments.



Comment at: clang/test/OpenMP/barrier_codegen.cpp:22
+// CLANGCG-NOT: readonly
+// IRBUILDER:  ; Function Attrs: nofree nosync nounwind readonly
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*)

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > Not sure about correct use of `nosync` and `readonly` attributes. 
> > > > > OpenMP runtime uses lazy initialization of the runtime library and 
> > > > > when any runtime function is called, the inner parts of the OpenMP 
> > > > > runtime are initialized automatically. It may use some sync 
> > > > > primitives and may modify memory, I assume. Same about `nofree`.
> > > > There are two versions of these functions, host and device. I assume 
> > > > host functions are not inlined but device functions might be. This is 
> > > > basically all the modes we support right now.
> > > > 
> > > > If we do not inline the function (host) we don't necessarily care what 
> > > > they do but what effect the user can expect.
> > > > The user can not expect to synchronize through 
> > > > `__kmpc_global_thread_num` calls in a defined way, thus `nosync`.
> > > > Similarly, from the users perspective there is no way to determine if 
> > > > something was written or freed, no matter how many of these calls I 
> > > > issue and under which control conditions, all I see is the number as a 
> > > > result. Thus, `readonly` and `nofree`. I believe `readnone` is even 
> > > > fine here but it might not work for the device version (see below) so I 
> > > > removed it.
> > > > 
> > > > If we do inline the function (device) we need to make sure the 
> > > > attributes are compatible with the inlined code to not cause UB. The 
> > > > code of `__kmpc_global_thread_num` at least does not write anything and 
> > > > only reads stuff (as far as I can tell).
> > > > 
> > > > Please correct me if I overlooked something. 
> > > But someone may try to inline the host-based runtime or try to use LTO 
> > > with it.
> > > The question is not about the user expectations but about the 
> > > optimizations which can be triggered with these attributes.
> > This is our runtime and we have supported and unsupported usage models.
> Hmm, I don't think this the right approach. Plus, you still did not answer 
> about optimizations. Maybe, currently, these attributes won't trigger 
> dangerous optimizations but they can do this in the future and it may lead to 
> unpredictable results. I would use the pessimistic model here rather than 
> over-optimistic.
I did (try to) describe why there cannot be any problems wrt. optimizations:
The specified behavior of the runtime call is _as if_ it is `readonly`, 
`nofree`, and `nosync`.
That is, from the perspective of the compiler this is true and optimizations 
are allowed to use that fact.
 
The fact that the first ever runtime call initializes the runtime is neither 
part of the specification nor of the observable behavior. If we change the 
order between two call to `__kmpc_global_thread_num`, or similar calls, we 
cannot observe if/which one initialized the runtime and which read only stuff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922



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

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/barrier_codegen.cpp:22
+// CLANGCG-NOT: readonly
+// IRBUILDER:  ; Function Attrs: nofree nosync nounwind readonly
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*)

jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > Not sure about correct use of `nosync` and `readonly` attributes. 
> > > > OpenMP runtime uses lazy initialization of the runtime library and when 
> > > > any runtime function is called, the inner parts of the OpenMP runtime 
> > > > are initialized automatically. It may use some sync primitives and may 
> > > > modify memory, I assume. Same about `nofree`.
> > > There are two versions of these functions, host and device. I assume host 
> > > functions are not inlined but device functions might be. This is 
> > > basically all the modes we support right now.
> > > 
> > > If we do not inline the function (host) we don't necessarily care what 
> > > they do but what effect the user can expect.
> > > The user can not expect to synchronize through `__kmpc_global_thread_num` 
> > > calls in a defined way, thus `nosync`.
> > > Similarly, from the users perspective there is no way to determine if 
> > > something was written or freed, no matter how many of these calls I issue 
> > > and under which control conditions, all I see is the number as a result. 
> > > Thus, `readonly` and `nofree`. I believe `readnone` is even fine here but 
> > > it might not work for the device version (see below) so I removed it.
> > > 
> > > If we do inline the function (device) we need to make sure the attributes 
> > > are compatible with the inlined code to not cause UB. The code of 
> > > `__kmpc_global_thread_num` at least does not write anything and only 
> > > reads stuff (as far as I can tell).
> > > 
> > > Please correct me if I overlooked something. 
> > But someone may try to inline the host-based runtime or try to use LTO with 
> > it.
> > The question is not about the user expectations but about the optimizations 
> > which can be triggered with these attributes.
> This is our runtime and we have supported and unsupported usage models.
Hmm, I don't think this the right approach. Plus, you still did not answer 
about optimizations. Maybe, currently, these attributes won't trigger dangerous 
optimizations but they can do this in the future and it may lead to 
unpredictable results. I would use the pessimistic model here rather than 
over-optimistic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922



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

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.



Comment at: clang/test/OpenMP/barrier_codegen.cpp:22
+// CLANGCG-NOT: readonly
+// IRBUILDER:  ; Function Attrs: nofree nosync nounwind readonly
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*)

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > Not sure about correct use of `nosync` and `readonly` attributes. OpenMP 
> > > runtime uses lazy initialization of the runtime library and when any 
> > > runtime function is called, the inner parts of the OpenMP runtime are 
> > > initialized automatically. It may use some sync primitives and may modify 
> > > memory, I assume. Same about `nofree`.
> > There are two versions of these functions, host and device. I assume host 
> > functions are not inlined but device functions might be. This is basically 
> > all the modes we support right now.
> > 
> > If we do not inline the function (host) we don't necessarily care what they 
> > do but what effect the user can expect.
> > The user can not expect to synchronize through `__kmpc_global_thread_num` 
> > calls in a defined way, thus `nosync`.
> > Similarly, from the users perspective there is no way to determine if 
> > something was written or freed, no matter how many of these calls I issue 
> > and under which control conditions, all I see is the number as a result. 
> > Thus, `readonly` and `nofree`. I believe `readnone` is even fine here but 
> > it might not work for the device version (see below) so I removed it.
> > 
> > If we do inline the function (device) we need to make sure the attributes 
> > are compatible with the inlined code to not cause UB. The code of 
> > `__kmpc_global_thread_num` at least does not write anything and only reads 
> > stuff (as far as I can tell).
> > 
> > Please correct me if I overlooked something. 
> But someone may try to inline the host-based runtime or try to use LTO with 
> it.
> The question is not about the user expectations but about the optimizations 
> which can be triggered with these attributes.
This is our runtime and we have supported and unsupported usage models.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922



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

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/barrier_codegen.cpp:22
+// CLANGCG-NOT: readonly
+// IRBUILDER:  ; Function Attrs: nofree nosync nounwind readonly
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*)

jdoerfert wrote:
> ABataev wrote:
> > Not sure about correct use of `nosync` and `readonly` attributes. OpenMP 
> > runtime uses lazy initialization of the runtime library and when any 
> > runtime function is called, the inner parts of the OpenMP runtime are 
> > initialized automatically. It may use some sync primitives and may modify 
> > memory, I assume. Same about `nofree`.
> There are two versions of these functions, host and device. I assume host 
> functions are not inlined but device functions might be. This is basically 
> all the modes we support right now.
> 
> If we do not inline the function (host) we don't necessarily care what they 
> do but what effect the user can expect.
> The user can not expect to synchronize through `__kmpc_global_thread_num` 
> calls in a defined way, thus `nosync`.
> Similarly, from the users perspective there is no way to determine if 
> something was written or freed, no matter how many of these calls I issue and 
> under which control conditions, all I see is the number as a result. Thus, 
> `readonly` and `nofree`. I believe `readnone` is even fine here but it might 
> not work for the device version (see below) so I removed it.
> 
> If we do inline the function (device) we need to make sure the attributes are 
> compatible with the inlined code to not cause UB. The code of 
> `__kmpc_global_thread_num` at least does not write anything and only reads 
> stuff (as far as I can tell).
> 
> Please correct me if I overlooked something. 
But someone may try to inline the host-based runtime or try to use LTO with it.
The question is not about the user expectations but about the optimizations 
which can be triggered with these attributes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922



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

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.



Comment at: clang/test/OpenMP/barrier_codegen.cpp:22
+// CLANGCG-NOT: readonly
+// IRBUILDER:  ; Function Attrs: nofree nosync nounwind readonly
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*)

ABataev wrote:
> Not sure about correct use of `nosync` and `readonly` attributes. OpenMP 
> runtime uses lazy initialization of the runtime library and when any runtime 
> function is called, the inner parts of the OpenMP runtime are initialized 
> automatically. It may use some sync primitives and may modify memory, I 
> assume. Same about `nofree`.
There are two versions of these functions, host and device. I assume host 
functions are not inlined but device functions might be. This is basically all 
the modes we support right now.

If we do not inline the function (host) we don't necessarily care what they do 
but what effect the user can expect.
The user can not expect to synchronize through `__kmpc_global_thread_num` calls 
in a defined way, thus `nosync`.
Similarly, from the users perspective there is no way to determine if something 
was written or freed, no matter how many of these calls I issue and under which 
control conditions, all I see is the number as a result. Thus, `readonly` and 
`nofree`. I believe `readnone` is even fine here but it might not work for the 
device version (see below) so I removed it.

If we do inline the function (device) we need to make sure the attributes are 
compatible with the inlined code to not cause UB. The code of 
`__kmpc_global_thread_num` at least does not write anything and only reads 
stuff (as far as I can tell).

Please correct me if I overlooked something. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922



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

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/barrier_codegen.cpp:22
+// CLANGCG-NOT: readonly
+// IRBUILDER:  ; Function Attrs: nofree nosync nounwind readonly
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*)

Not sure about correct use of `nosync` and `readonly` attributes. OpenMP 
runtime uses lazy initialization of the runtime library and when any runtime 
function is called, the inner parts of the OpenMP runtime are initialized 
automatically. It may use some sync primitives and may modify memory, I assume. 
Same about `nofree`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922



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

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 228331.
jdoerfert marked 2 inline comments as done.
jdoerfert added a comment.
Herald added a project: LLVM.

Adjust tests as requested


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/fopenmp.c
  clang/test/OpenMP/barrier_codegen.cpp
  llvm/include/llvm/IR/OpenMPIRBuilder.h

Index: llvm/include/llvm/IR/OpenMPIRBuilder.h
===
--- llvm/include/llvm/IR/OpenMPIRBuilder.h
+++ llvm/include/llvm/IR/OpenMPIRBuilder.h
@@ -37,6 +37,9 @@
   /// Description of a LLVM-IR insertion point (IP) and a debug/source location
   /// (filename, line, column, ...).
   struct LocationDescription {
+template 
+LocationDescription(const IRBuilder )
+: IP(IRB.saveIP()), DL(IRB.getCurrentDebugLocation()) {}
 LocationDescription(const IRBuilder<>::InsertPoint ) : IP(IP) {}
 LocationDescription(const IRBuilder<>::InsertPoint , const DebugLoc )
 : IP(IP), DL(DL) {}
Index: clang/test/OpenMP/barrier_codegen.cpp
===
--- clang/test/OpenMP/barrier_codegen.cpp
+++ clang/test/OpenMP/barrier_codegen.cpp
@@ -1,6 +1,10 @@
-// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,CLANGCG
 // RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-pch -o %t %s
-// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,CLANGCG
+
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER
 
 // RUN: %clang_cc1 -verify -fopenmp-simd -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck --check-prefix SIMD-ONLY0 %s
 // RUN: %clang_cc1 -fopenmp-simd -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-pch -o %t %s
@@ -14,6 +18,10 @@
 // CHECK-DAG: [[EXPLICIT_BARRIER_LOC:@.+]] = {{.+}} [[IDENT_T]] { i32 0, i32 34, i32 0, i32 0, i8* getelementptr inbounds ([{{[0-9]+}} x i8], [{{[0-9]+}} x i8]* @{{.+}}, i32 0, i32 0) }
 // CHECK-DAG: [[LOC:@.+]] = {{.+}} [[IDENT_T]] { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([{{[0-9]+}} x i8], [{{[0-9]+}} x i8]* @{{.+}}, i32 0, i32 0) }
 
+// CLANGCG-NOT: readonly
+// IRBUILDER:  ; Function Attrs: nofree nosync nounwind readonly
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*)
+
 void foo() {}
 
 template 
Index: clang/test/Driver/fopenmp.c
===
--- clang/test/Driver/fopenmp.c
+++ clang/test/Driver/fopenmp.c
@@ -124,6 +124,12 @@
 // CHECK-LD-STATIC-IOMP5-NO-BDYNAMIC: "-{{B?}}static" {{.*}} "-liomp5"
 // CHECK-LD-STATIC-IOMP5-NO-BDYNAMIC-NOT: "-Bdynamic"
 //
+// RUN: %clang -target x86_64-linux-gnu -fopenmp -fopenmp-enable-irbuilder -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CC1-OPENMPIRBUILDER
+//
+// CHECK-CC1-OPENMPIRBUILDER: "-cc1"
+// CHECK-CC1-OPENMPIRBUILDER-SAME: "-fopenmp"
+// CHECK-CC1-OPENMPIRBUILDER-SAME: "-fopenmp-enable-irbuilder"
+//
 // We'd like to check that the default is sane, but until we have the ability
 // to *always* semantically analyze OpenMP without always generating runtime
 // calls (in the event of an unsupported runtime), we don't have a good way to
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2996,6 +2996,8 @@
   Opts.OpenMP && !Args.hasArg(options::OPT_fnoopenmp_use_tls);
   Opts.OpenMPIsDevice =
   Opts.OpenMP && Args.hasArg(options::OPT_fopenmp_is_device);
+  Opts.OpenMPIRBuilder =
+  Opts.OpenMP && Args.hasArg(options::OPT_fopenmp_enable_irbuilder);
   bool IsTargetSpecified =
   Opts.OpenMPIsDevice || 

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 4 inline comments as done.
jdoerfert added inline comments.



Comment at: clang/test/Driver/fopenmp.c:130-131
+// CHECK-CC1-OPENMPIRBUILDER: "-cc1"
+// CHECK-CC1-OPENMPIRBUILDER: "-fopenmp"
+// CHECK-CC1-OPENMPIRBUILDER: "-fopenmp-enable-irbuilder"
+//

ABataev wrote:
> `CHECK...-SAME`?
Sure, will do.



Comment at: clang/test/OpenMP/barrier_codegen.cpp:42
+// CLANGCG-NOT: inaccessiblemem
+// IRBUILDER:  ; Function Attrs: inaccessiblemem_or_argmemonly nofree 
nosync nounwind readonly
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*)

ABataev wrote:
> Not sure about correctness of `inaccessiblemem_or_argmemonly` attribute 
> applied to this function. Are you sure about this? This is maybe true for the 
> NVPTX/AMDGCN runtimes but not the generic version of libomp.
Fair point. I'll remove it for now but we should come up with a scheme. Maybe 
communicating the target library and other information to the IRBuilder so we 
can select based on those.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922



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

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/Driver/fopenmp.c:130-131
+// CHECK-CC1-OPENMPIRBUILDER: "-cc1"
+// CHECK-CC1-OPENMPIRBUILDER: "-fopenmp"
+// CHECK-CC1-OPENMPIRBUILDER: "-fopenmp-enable-irbuilder"
+//

`CHECK...-SAME`?



Comment at: clang/test/OpenMP/barrier_codegen.cpp:42
+// CLANGCG-NOT: inaccessiblemem
+// IRBUILDER:  ; Function Attrs: inaccessiblemem_or_argmemonly nofree 
nosync nounwind readonly
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*)

Not sure about correctness of `inaccessiblemem_or_argmemonly` attribute applied 
to this function. Are you sure about this? This is maybe true for the 
NVPTX/AMDGCN runtimes but not the generic version of libomp.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922



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

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 228281.
jdoerfert added a comment.

Add driver test coverage


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/fopenmp.c
  clang/test/OpenMP/barrier_codegen.cpp

Index: clang/test/OpenMP/barrier_codegen.cpp
===
--- clang/test/OpenMP/barrier_codegen.cpp
+++ clang/test/OpenMP/barrier_codegen.cpp
@@ -1,6 +1,10 @@
-// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,CLANGCG
 // RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-pch -o %t %s
-// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,CLANGCG
+
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER
 
 // RUN: %clang_cc1 -verify -fopenmp-simd -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck --check-prefix SIMD-ONLY0 %s
 // RUN: %clang_cc1 -fopenmp-simd -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-pch -o %t %s
@@ -34,6 +38,10 @@
   return tmain(argc) + tmain(argv[0][0]) + a;
 }
 
+// CLANGCG-NOT: inaccessiblemem
+// IRBUILDER:  ; Function Attrs: inaccessiblemem_or_argmemonly nofree nosync nounwind readonly
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*)
+
 // CHECK: define {{.+}} [[TMAIN_INT]](
 // CHECK: [[GTID:%.+]] = call i32 @__kmpc_global_thread_num([[IDENT_T]]* [[LOC]])
 // CHECK: call void @__kmpc_barrier([[IDENT_T]]* [[EXPLICIT_BARRIER_LOC]], i32 [[GTID]])
Index: clang/test/Driver/fopenmp.c
===
--- clang/test/Driver/fopenmp.c
+++ clang/test/Driver/fopenmp.c
@@ -124,6 +124,12 @@
 // CHECK-LD-STATIC-IOMP5-NO-BDYNAMIC: "-{{B?}}static" {{.*}} "-liomp5"
 // CHECK-LD-STATIC-IOMP5-NO-BDYNAMIC-NOT: "-Bdynamic"
 //
+// RUN: %clang -target x86_64-linux-gnu -fopenmp -fopenmp-enable-irbuilder -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CC1-OPENMPIRBUILDER
+//
+// CHECK-CC1-OPENMPIRBUILDER: "-cc1"
+// CHECK-CC1-OPENMPIRBUILDER: "-fopenmp"
+// CHECK-CC1-OPENMPIRBUILDER: "-fopenmp-enable-irbuilder"
+//
 // We'd like to check that the default is sane, but until we have the ability
 // to *always* semantically analyze OpenMP without always generating runtime
 // calls (in the event of an unsupported runtime), we don't have a good way to
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2996,6 +2996,8 @@
   Opts.OpenMP && !Args.hasArg(options::OPT_fnoopenmp_use_tls);
   Opts.OpenMPIsDevice =
   Opts.OpenMP && Args.hasArg(options::OPT_fopenmp_is_device);
+  Opts.OpenMPIRBuilder =
+  Opts.OpenMP && Args.hasArg(options::OPT_fopenmp_enable_irbuilder);
   bool IsTargetSpecified =
   Opts.OpenMPIsDevice || Args.hasArg(options::OPT_fopenmp_targets_EQ);
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4555,6 +4555,7 @@
 CmdArgs.push_back("-fnoopenmp-use-tls");
   Args.AddLastArg(CmdArgs, options::OPT_fopenmp_simd,
   options::OPT_fno_openmp_simd);
+  Args.AddAllArgs(CmdArgs, options::OPT_fopenmp_enable_irbuilder);
   Args.AddAllArgs(CmdArgs, options::OPT_fopenmp_version_EQ);
   Args.AddAllArgs(CmdArgs, options::OPT_fopenmp_cuda_number_of_sm_EQ);
   Args.AddAllArgs(CmdArgs, options::OPT_fopenmp_cuda_blocks_per_sm_EQ);
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1643-1644
   HelpText<"Emit OpenMP code only for SIMD-based constructs.">;
+def fopenmp_enable_irbuilder : Flag<["-"], "fopenmp-enable-irbuilder">, 
Group, Flags<[CC1Option, NoArgumentUnused, HelpHidden]>,
+  HelpText<"Use the experimental OpenMP-IR-Builder codegen path.">;
 def fno_openmp_simd : Flag<["-"], "fno-openmp-simd">, Group, 
Flags<[CC1Option, NoArgumentUnused]>;

jdoerfert wrote:
> ABataev wrote:
> > Do we need to expose this option to driver or it is enough to have just a 
> > frontend option? If still need to have a driver option, add a test for 
> > driver.
> How do I write a frontend option? Anything that we can query in the 
> lib/CodeGen is fine with me. (I don't even need an option if we turn this on 
> by default to get test coverage right away).
You nedd to move to CC1Options.td file. It means that you can't use it direcly 
when invoke the drive, instead you will need to use `-Xclang -fopenmp-...`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922



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

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1643-1644
   HelpText<"Emit OpenMP code only for SIMD-based constructs.">;
+def fopenmp_enable_irbuilder : Flag<["-"], "fopenmp-enable-irbuilder">, 
Group, Flags<[CC1Option, NoArgumentUnused, HelpHidden]>,
+  HelpText<"Use the experimental OpenMP-IR-Builder codegen path.">;
 def fno_openmp_simd : Flag<["-"], "fno-openmp-simd">, Group, 
Flags<[CC1Option, NoArgumentUnused]>;

ABataev wrote:
> Do we need to expose this option to driver or it is enough to have just a 
> frontend option? If still need to have a driver option, add a test for driver.
How do I write a frontend option? Anything that we can query in the lib/CodeGen 
is fine with me. (I don't even need an option if we turn this on by default to 
get test coverage right away).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922



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

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 228270.
jdoerfert added a comment.

Update test attributes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/OpenMP/barrier_codegen.cpp

Index: clang/test/OpenMP/barrier_codegen.cpp
===
--- clang/test/OpenMP/barrier_codegen.cpp
+++ clang/test/OpenMP/barrier_codegen.cpp
@@ -1,6 +1,10 @@
-// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,CLANGCG
 // RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-pch -o %t %s
-// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,CLANGCG
+
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER
 
 // RUN: %clang_cc1 -verify -fopenmp-simd -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck --check-prefix SIMD-ONLY0 %s
 // RUN: %clang_cc1 -fopenmp-simd -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-pch -o %t %s
@@ -34,6 +38,10 @@
   return tmain(argc) + tmain(argv[0][0]) + a;
 }
 
+// CLANGCG-NOT: inaccessiblemem
+// IRBUILDER:  ; Function Attrs: inaccessiblemem_or_argmemonly nofree nosync nounwind readonly
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*)
+
 // CHECK: define {{.+}} [[TMAIN_INT]](
 // CHECK: [[GTID:%.+]] = call i32 @__kmpc_global_thread_num([[IDENT_T]]* [[LOC]])
 // CHECK: call void @__kmpc_barrier([[IDENT_T]]* [[EXPLICIT_BARRIER_LOC]], i32 [[GTID]])
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2996,6 +2996,8 @@
   Opts.OpenMP && !Args.hasArg(options::OPT_fnoopenmp_use_tls);
   Opts.OpenMPIsDevice =
   Opts.OpenMP && Args.hasArg(options::OPT_fopenmp_is_device);
+  Opts.OpenMPIRBuilder =
+  Opts.OpenMP && Args.hasArg(options::OPT_fopenmp_enable_irbuilder);
   bool IsTargetSpecified =
   Opts.OpenMPIsDevice || Args.hasArg(options::OPT_fopenmp_targets_EQ);
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4555,6 +4555,7 @@
 CmdArgs.push_back("-fnoopenmp-use-tls");
   Args.AddLastArg(CmdArgs, options::OPT_fopenmp_simd,
   options::OPT_fno_openmp_simd);
+  Args.AddAllArgs(CmdArgs, options::OPT_fopenmp_enable_irbuilder);
   Args.AddAllArgs(CmdArgs, options::OPT_fopenmp_version_EQ);
   Args.AddAllArgs(CmdArgs, options::OPT_fopenmp_cuda_number_of_sm_EQ);
   Args.AddAllArgs(CmdArgs, options::OPT_fopenmp_cuda_blocks_per_sm_EQ);
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -45,6 +45,7 @@
 class DataLayout;
 class FunctionType;
 class LLVMContext;
+class OpenMPIRBuilder;
 class IndexedInstrProfReader;
 }
 
@@ -319,6 +320,7 @@
   std::unique_ptr ObjCRuntime;
   std::unique_ptr OpenCLRuntime;
   std::unique_ptr OpenMPRuntime;
+  std::unique_ptr OMPBuilder;
   std::unique_ptr CUDARuntime;
   std::unique_ptr DebugInfo;
   std::unique_ptr ObjCData;
@@ -585,6 +587,9 @@
 return *OpenMPRuntime;
   }
 
+  /// Return a pointer to the configured OpenMPIRBuilder, if any.
+  llvm::OpenMPIRBuilder *getOpenMPIRBuilder() { return OMPBuilder.get(); }
+
   /// Return a reference to the configured CUDA runtime.
   CGCUDARuntime () {
 assert(CUDARuntime != nullptr);
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ 

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1643-1644
   HelpText<"Emit OpenMP code only for SIMD-based constructs.">;
+def fopenmp_enable_irbuilder : Flag<["-"], "fopenmp-enable-irbuilder">, 
Group, Flags<[CC1Option, NoArgumentUnused, HelpHidden]>,
+  HelpText<"Use the experimental OpenMP-IR-Builder codegen path.">;
 def fno_openmp_simd : Flag<["-"], "fno-openmp-simd">, Group, 
Flags<[CC1Option, NoArgumentUnused]>;

Do we need to expose this option to driver or it is enough to have just a 
frontend option? If still need to have a driver option, add a test for driver.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922



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

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 228264.
jdoerfert added a comment.

Make the command line option shorter (mentioning openmp once should suffice)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/OpenMP/barrier_codegen.cpp

Index: clang/test/OpenMP/barrier_codegen.cpp
===
--- clang/test/OpenMP/barrier_codegen.cpp
+++ clang/test/OpenMP/barrier_codegen.cpp
@@ -1,6 +1,10 @@
-// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,CLANGCG
 // RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-pch -o %t %s
-// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,CLANGCG
+
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-irbuilder -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER
 
 // RUN: %clang_cc1 -verify -fopenmp-simd -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck --check-prefix SIMD-ONLY0 %s
 // RUN: %clang_cc1 -fopenmp-simd -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-pch -o %t %s
@@ -34,6 +38,10 @@
   return tmain(argc) + tmain(argv[0][0]) + a;
 }
 
+// CLANGCG-NOT: inaccessiblemem_or_argmemonly
+// IRBUILDER:  ; Function Attrs: inaccessiblemem_or_argmemonly
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*) #2
+
 // CHECK: define {{.+}} [[TMAIN_INT]](
 // CHECK: [[GTID:%.+]] = call i32 @__kmpc_global_thread_num([[IDENT_T]]* [[LOC]])
 // CHECK: call void @__kmpc_barrier([[IDENT_T]]* [[EXPLICIT_BARRIER_LOC]], i32 [[GTID]])
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2996,6 +2996,8 @@
   Opts.OpenMP && !Args.hasArg(options::OPT_fnoopenmp_use_tls);
   Opts.OpenMPIsDevice =
   Opts.OpenMP && Args.hasArg(options::OPT_fopenmp_is_device);
+  Opts.OpenMPIRBuilder =
+  Opts.OpenMP && Args.hasArg(options::OPT_fopenmp_enable_irbuilder);
   bool IsTargetSpecified =
   Opts.OpenMPIsDevice || Args.hasArg(options::OPT_fopenmp_targets_EQ);
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4555,6 +4555,7 @@
 CmdArgs.push_back("-fnoopenmp-use-tls");
   Args.AddLastArg(CmdArgs, options::OPT_fopenmp_simd,
   options::OPT_fno_openmp_simd);
+  Args.AddAllArgs(CmdArgs, options::OPT_fopenmp_enable_irbuilder);
   Args.AddAllArgs(CmdArgs, options::OPT_fopenmp_version_EQ);
   Args.AddAllArgs(CmdArgs, options::OPT_fopenmp_cuda_number_of_sm_EQ);
   Args.AddAllArgs(CmdArgs, options::OPT_fopenmp_cuda_blocks_per_sm_EQ);
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -45,6 +45,7 @@
 class DataLayout;
 class FunctionType;
 class LLVMContext;
+class OpenMPIRBuilder;
 class IndexedInstrProfReader;
 }
 
@@ -319,6 +320,7 @@
   std::unique_ptr ObjCRuntime;
   std::unique_ptr OpenCLRuntime;
   std::unique_ptr OpenMPRuntime;
+  std::unique_ptr OMPBuilder;
   std::unique_ptr CUDARuntime;
   std::unique_ptr DebugInfo;
   std::unique_ptr ObjCData;
@@ -585,6 +587,9 @@
 return *OpenMPRuntime;
   }
 
+  /// Return a pointer to the configured OpenMPIRBuilder, if any.
+  llvm::OpenMPIRBuilder *getOpenMPIRBuilder() { return OMPBuilder.get(); }
+
   /// Return a reference to the configured CUDA runtime.
   CGCUDARuntime () {
 assert(CUDARuntime != nullptr);
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- 

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3489
+ !OMPRegionInfo->hasCancel())) {
+OMPBuilder->CreateBarrier({CGF.Builder.saveIP()}, Kind, ForceSimpleCall,
+  EmitChecks);

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > `createBarrier`
> > I'd say we align ourselves with the IRBuilder (which has `CreateXXX` 
> > methods) or we are different enough to avoid confusion, e.g., `emit`. I 
> > don't care much which way but `create` for one builder and `Create` 
> > for another will be confusing, don't you think?
> Shall follow the LLVM coding document and format the new function name in 
> accordance with it?
Changing the IRBuilder is something no one dared to do for years. I'm actually 
in favor but I would like to get this in rather sooner than later.

Two options I think are good:
  - Use `CreateXXX` now and, if someone ever comes around to rename the Builder 
methods we rename them in all Builders.
  - Use `emit` now to avoid confusion.

When someone in a review before asked for create instead of emit the reason was 
to be aligned with the IRBuilder.  I think that is a good justification so I 
went with `CreateXXX` even if it is against the coding standard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922



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

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3489
+ !OMPRegionInfo->hasCancel())) {
+OMPBuilder->CreateBarrier({CGF.Builder.saveIP()}, Kind, ForceSimpleCall,
+  EmitChecks);

jdoerfert wrote:
> ABataev wrote:
> > `createBarrier`
> I'd say we align ourselves with the IRBuilder (which has `CreateXXX` methods) 
> or we are different enough to avoid confusion, e.g., `emit`. I don't care 
> much which way but `create` for one builder and `Create` for another 
> will be confusing, don't you think?
Shall follow the LLVM coding document and format the new function name in 
accordance with it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922



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

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 2 inline comments as done.
jdoerfert added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3489
+ !OMPRegionInfo->hasCancel())) {
+OMPBuilder->CreateBarrier({CGF.Builder.saveIP()}, Kind, ForceSimpleCall,
+  EmitChecks);

ABataev wrote:
> `createBarrier`
I'd say we align ourselves with the IRBuilder (which has `CreateXXX` methods) 
or we are different enough to avoid confusion, e.g., `emit`. I don't care 
much which way but `create` for one builder and `Create` for another 
will be confusing, don't you think?



Comment at: clang/test/OpenMP/barrier_codegen.cpp:43
+// IRBUILDER:  ; Function Attrs: inaccessiblemem_or_argmemonly
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*) #2
+

ABataev wrote:
> Remove `#2`, it may be changed 
Agreed, I even added an option to the update_test script to do so but forgot to 
use it (D68851).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922



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

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3489
+ !OMPRegionInfo->hasCancel())) {
+OMPBuilder->CreateBarrier({CGF.Builder.saveIP()}, Kind, ForceSimpleCall,
+  EmitChecks);

`createBarrier`



Comment at: clang/test/OpenMP/barrier_codegen.cpp:43
+// IRBUILDER:  ; Function Attrs: inaccessiblemem_or_argmemonly
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*) #2
+

Remove `#2`, it may be changed 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69922



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

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert created this revision.
jdoerfert added reviewers: kiranchandramohan, ABataev, RaviNarayanaswamy, 
gtbercea, grokos, sdmitriev, JonChesterfield, hfinkel, fghanim.
Herald added a project: clang.

This is the initial patch to use the OpenMP-IR-Builder, as discussed on
the mailing list ([1] and later) and at the US Dev Meeting'19.

The design is similar to D61953  but:

- placed in `llvm/lib/IR/` next to IRBuilder, for lack of a better location.
- in a non-WIP status, with proper documentation and working.
- using a OpenMPKinds.def file to manage lists of directives, runtime 
functions, types, ..., similar to the current Clang implementation.
- restricted to handle only (simple) barriers, to implement most `#pragma omp 
barrier` directives and most implicit barriers.
- properly hooked into Clang to be used if possible.
- compatible with the remaining code generation.

Parts have been extracted into D69853  and 
D69785 .

The plan is to have multiple people working on moving logic from Clang
here once the initial scaffolding landed.

[1] 
http://lists.flang-compiler.org/pipermail/flang-dev_lists.flang-compiler.org/2019-May/000197.html


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69922

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/OpenMP/barrier_codegen.cpp

Index: clang/test/OpenMP/barrier_codegen.cpp
===
--- clang/test/OpenMP/barrier_codegen.cpp
+++ clang/test/OpenMP/barrier_codegen.cpp
@@ -1,6 +1,10 @@
-// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,CLANGCG
 // RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-pch -o %t %s
-// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,CLANGCG
+
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-openmpirbuilder -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -fopenmp-enable-openmpirbuilder -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -fopenmp-enable-openmpirbuilder -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,IRBUILDER
 
 // RUN: %clang_cc1 -verify -fopenmp-simd -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck --check-prefix SIMD-ONLY0 %s
 // RUN: %clang_cc1 -fopenmp-simd -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-pch -o %t %s
@@ -34,6 +38,10 @@
   return tmain(argc) + tmain(argv[0][0]) + a;
 }
 
+// CLANGCG-NOT: inaccessiblemem_or_argmemonly
+// IRBUILDER:  ; Function Attrs: inaccessiblemem_or_argmemonly
+// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*) #2
+
 // CHECK: define {{.+}} [[TMAIN_INT]](
 // CHECK: [[GTID:%.+]] = call i32 @__kmpc_global_thread_num([[IDENT_T]]* [[LOC]])
 // CHECK: call void @__kmpc_barrier([[IDENT_T]]* [[EXPLICIT_BARRIER_LOC]], i32 [[GTID]])
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2996,6 +2996,8 @@
   Opts.OpenMP && !Args.hasArg(options::OPT_fnoopenmp_use_tls);
   Opts.OpenMPIsDevice =
   Opts.OpenMP && Args.hasArg(options::OPT_fopenmp_is_device);
+  Opts.OpenMPIRBuilder =
+  Opts.OpenMP && Args.hasArg(options::OPT_fopenmp_enable_openmpirbuilder);
   bool IsTargetSpecified =
   Opts.OpenMPIsDevice || Args.hasArg(options::OPT_fopenmp_targets_EQ);
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4555,6 +4555,7 @@
 CmdArgs.push_back("-fnoopenmp-use-tls");
   Args.AddLastArg(CmdArgs, options::OPT_fopenmp_simd,
   options::OPT_fno_openmp_simd);
+  Args.AddAllArgs(CmdArgs, options::OPT_fopenmp_enable_openmpirbuilder);
   Args.AddAllArgs(CmdArgs, options::OPT_fopenmp_version_EQ);
   Args.AddAllArgs(CmdArgs, options::OPT_fopenmp_cuda_number_of_sm_EQ);
   Args.AddAllArgs(CmdArgs,