[PATCH] D107649: [OpenMP]Fix PR51349: Remove AlwaysInline for if regions.

2021-08-06 Thread Joseph Huber via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG41a6b50c2596: [OpenMP]Fix PR51349: Remove AlwaysInline for 
if regions. (authored by jhuber6).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107649

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/test/OpenMP/parallel_if_codegen_PR51349.cpp


Index: clang/test/OpenMP/parallel_if_codegen_PR51349.cpp
===
--- /dev/null
+++ clang/test/OpenMP/parallel_if_codegen_PR51349.cpp
@@ -0,0 +1,38 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py 
UTC_ARGS: --function-signature --check-attributes --include-generated-funcs
+// RUN: %clang_cc1 -x c++ -O1 -fopenmp-version=45 -disable-llvm-optzns -verify 
-fopenmp -triple x86_64-unknown-linux -emit-llvm %s -o - | FileCheck %s 
--check-prefix=CHECK
+// expected-no-diagnostics
+
+#ifndef HEADER
+#define HEADER
+
+void foo() {
+#pragma omp parallel if(0)
+  ;
+}
+
+#endif
+// CHECK: Function Attrs: mustprogress nounwind
+// CHECK-LABEL: define {{[^@]+}}@_Z3foov
+// CHECK-SAME: () #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[DOTTHREADID_TEMP_:%.*]] = alloca i32, align 4
+// CHECK-NEXT:[[DOTBOUND_ZERO_ADDR:%.*]] = alloca i32, align 4
+// CHECK-NEXT:store i32 0, i32* [[DOTBOUND_ZERO_ADDR]], align 4
+// CHECK-NEXT:[[TMP0:%.*]] = call i32 
@__kmpc_global_thread_num(%struct.ident_t* @[[GLOB1:[0-9]+]])
+// CHECK-NEXT:call void @__kmpc_serialized_parallel(%struct.ident_t* 
@[[GLOB1]], i32 [[TMP0]])
+// CHECK-NEXT:store i32 [[TMP0]], i32* [[DOTTHREADID_TEMP_]], align 4, 
!tbaa [[TBAA3:![0-9]+]]
+// CHECK-NEXT:call void @.omp_outlined.(i32* [[DOTTHREADID_TEMP_]], i32* 
[[DOTBOUND_ZERO_ADDR]]) #[[ATTR2:[0-9]+]]
+// CHECK-NEXT:call void @__kmpc_end_serialized_parallel(%struct.ident_t* 
@[[GLOB1]], i32 [[TMP0]])
+// CHECK-NEXT:ret void
+//
+//
+// CHECK: Function Attrs: noinline norecurse nounwind
+// CHECK-LABEL: define {{[^@]+}}@.omp_outlined.
+// CHECK-SAME: (i32* noalias [[DOTGLOBAL_TID_:%.*]], i32* noalias 
[[DOTBOUND_TID_:%.*]]) #[[ATTR1:[0-9]+]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[DOTGLOBAL_TID__ADDR:%.*]] = alloca i32*, align 8
+// CHECK-NEXT:[[DOTBOUND_TID__ADDR:%.*]] = alloca i32*, align 8
+// CHECK-NEXT:store i32* [[DOTGLOBAL_TID_]], i32** 
[[DOTGLOBAL_TID__ADDR]], align 8, !tbaa [[TBAA7:![0-9]+]]
+// CHECK-NEXT:store i32* [[DOTBOUND_TID_]], i32** [[DOTBOUND_TID__ADDR]], 
align 8, !tbaa [[TBAA7]]
+// CHECK-NEXT:ret void
+//
Index: clang/lib/CodeGen/CGOpenMPRuntime.cpp
===
--- clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -2120,11 +2120,12 @@
 OutlinedFnArgs.append(CapturedVars.begin(), CapturedVars.end());
 
 // Ensure we do not inline the function. This is trivially true for the 
ones
-// passed to __kmpc_fork_call but the ones calles in serialized regions
+// passed to __kmpc_fork_call but the ones called in serialized regions
 // could be inlined. This is not a perfect but it is closer to the 
invariant
 // we want, namely, every data environment starts with a new function.
 // TODO: We should pass the if condition to the runtime function and do the
 //   handling there. Much cleaner code.
+OutlinedFn->removeFnAttr(llvm::Attribute::AlwaysInline);
 OutlinedFn->addFnAttr(llvm::Attribute::NoInline);
 RT.emitOutlinedFunctionCall(CGF, Loc, OutlinedFn, OutlinedFnArgs);
 


Index: clang/test/OpenMP/parallel_if_codegen_PR51349.cpp
===
--- /dev/null
+++ clang/test/OpenMP/parallel_if_codegen_PR51349.cpp
@@ -0,0 +1,38 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --function-signature --check-attributes --include-generated-funcs
+// RUN: %clang_cc1 -x c++ -O1 -fopenmp-version=45 -disable-llvm-optzns -verify -fopenmp -triple x86_64-unknown-linux -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK
+// expected-no-diagnostics
+
+#ifndef HEADER
+#define HEADER
+
+void foo() {
+#pragma omp parallel if(0)
+  ;
+}
+
+#endif
+// CHECK: Function Attrs: mustprogress nounwind
+// CHECK-LABEL: define {{[^@]+}}@_Z3foov
+// CHECK-SAME: () #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[DOTTHREADID_TEMP_:%.*]] = alloca i32, align 4
+// CHECK-NEXT:[[DOTBOUND_ZERO_ADDR:%.*]] = alloca i32, align 4
+// CHECK-NEXT:store i32 0, i32* [[DOTBOUND_ZERO_ADDR]], align 4
+// CHECK-NEXT:[[TMP0:%.*]] = call i32 @__kmpc_global_thread_num(%struct.ident_t* @[[GLOB1:[0-9]+]])
+// CHECK-NEXT:call void @__kmpc_serialized_parallel(%struct.ident_t* @[[GLOB1]], i32 [[TMP0]])
+// CHECK-NEXT:store i32 [[TMP0]], i32* [[DOTTHREADID_TEMP_]], align 4, !tbaa [[TBAA3:![0-9]+]]
+//

[PATCH] D107649: [OpenMP]Fix PR51349: Remove AlwaysInline for if regions.

2021-08-06 Thread Mike Rice via Phabricator via cfe-commits 
mikerice accepted this revision.
mikerice added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107649

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

[PATCH] D107649: [OpenMP]Fix PR51349: Remove AlwaysInline for if regions.

2021-08-06 Thread Joseph Huber via Phabricator via cfe-commits 
jhuber6 updated this revision to Diff 364844.
jhuber6 added a comment.

Adding test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107649

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/test/OpenMP/parallel_if_codegen_PR51349.cpp


Index: clang/test/OpenMP/parallel_if_codegen_PR51349.cpp
===
--- /dev/null
+++ clang/test/OpenMP/parallel_if_codegen_PR51349.cpp
@@ -0,0 +1,38 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py 
UTC_ARGS: --function-signature --check-attributes --include-generated-funcs
+// RUN: %clang_cc1 -x c++ -O1 -fopenmp-version=45 -disable-llvm-optzns -verify 
-fopenmp -triple x86_64-unknown-linux -emit-llvm %s -o - | FileCheck %s 
--check-prefix=CHECK
+// expected-no-diagnostics
+
+#ifndef HEADER
+#define HEADER
+
+void foo() {
+#pragma omp parallel if(0)
+  ;
+}
+
+#endif
+// CHECK: Function Attrs: mustprogress nounwind
+// CHECK-LABEL: define {{[^@]+}}@_Z3foov
+// CHECK-SAME: () #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[DOTTHREADID_TEMP_:%.*]] = alloca i32, align 4
+// CHECK-NEXT:[[DOTBOUND_ZERO_ADDR:%.*]] = alloca i32, align 4
+// CHECK-NEXT:store i32 0, i32* [[DOTBOUND_ZERO_ADDR]], align 4
+// CHECK-NEXT:[[TMP0:%.*]] = call i32 
@__kmpc_global_thread_num(%struct.ident_t* @[[GLOB1:[0-9]+]])
+// CHECK-NEXT:call void @__kmpc_serialized_parallel(%struct.ident_t* 
@[[GLOB1]], i32 [[TMP0]])
+// CHECK-NEXT:store i32 [[TMP0]], i32* [[DOTTHREADID_TEMP_]], align 4, 
!tbaa [[TBAA3:![0-9]+]]
+// CHECK-NEXT:call void @.omp_outlined.(i32* [[DOTTHREADID_TEMP_]], i32* 
[[DOTBOUND_ZERO_ADDR]]) #[[ATTR2:[0-9]+]]
+// CHECK-NEXT:call void @__kmpc_end_serialized_parallel(%struct.ident_t* 
@[[GLOB1]], i32 [[TMP0]])
+// CHECK-NEXT:ret void
+//
+//
+// CHECK: Function Attrs: noinline norecurse nounwind
+// CHECK-LABEL: define {{[^@]+}}@.omp_outlined.
+// CHECK-SAME: (i32* noalias [[DOTGLOBAL_TID_:%.*]], i32* noalias 
[[DOTBOUND_TID_:%.*]]) #[[ATTR1:[0-9]+]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[DOTGLOBAL_TID__ADDR:%.*]] = alloca i32*, align 8
+// CHECK-NEXT:[[DOTBOUND_TID__ADDR:%.*]] = alloca i32*, align 8
+// CHECK-NEXT:store i32* [[DOTGLOBAL_TID_]], i32** 
[[DOTGLOBAL_TID__ADDR]], align 8, !tbaa [[TBAA7:![0-9]+]]
+// CHECK-NEXT:store i32* [[DOTBOUND_TID_]], i32** [[DOTBOUND_TID__ADDR]], 
align 8, !tbaa [[TBAA7]]
+// CHECK-NEXT:ret void
+//
Index: clang/lib/CodeGen/CGOpenMPRuntime.cpp
===
--- clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -2120,11 +2120,12 @@
 OutlinedFnArgs.append(CapturedVars.begin(), CapturedVars.end());
 
 // Ensure we do not inline the function. This is trivially true for the 
ones
-// passed to __kmpc_fork_call but the ones calles in serialized regions
+// passed to __kmpc_fork_call but the ones called in serialized regions
 // could be inlined. This is not a perfect but it is closer to the 
invariant
 // we want, namely, every data environment starts with a new function.
 // TODO: We should pass the if condition to the runtime function and do the
 //   handling there. Much cleaner code.
+OutlinedFn->removeFnAttr(llvm::Attribute::AlwaysInline);
 OutlinedFn->addFnAttr(llvm::Attribute::NoInline);
 RT.emitOutlinedFunctionCall(CGF, Loc, OutlinedFn, OutlinedFnArgs);
 


Index: clang/test/OpenMP/parallel_if_codegen_PR51349.cpp
===
--- /dev/null
+++ clang/test/OpenMP/parallel_if_codegen_PR51349.cpp
@@ -0,0 +1,38 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --function-signature --check-attributes --include-generated-funcs
+// RUN: %clang_cc1 -x c++ -O1 -fopenmp-version=45 -disable-llvm-optzns -verify -fopenmp -triple x86_64-unknown-linux -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK
+// expected-no-diagnostics
+
+#ifndef HEADER
+#define HEADER
+
+void foo() {
+#pragma omp parallel if(0)
+  ;
+}
+
+#endif
+// CHECK: Function Attrs: mustprogress nounwind
+// CHECK-LABEL: define {{[^@]+}}@_Z3foov
+// CHECK-SAME: () #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[DOTTHREADID_TEMP_:%.*]] = alloca i32, align 4
+// CHECK-NEXT:[[DOTBOUND_ZERO_ADDR:%.*]] = alloca i32, align 4
+// CHECK-NEXT:store i32 0, i32* [[DOTBOUND_ZERO_ADDR]], align 4
+// CHECK-NEXT:[[TMP0:%.*]] = call i32 @__kmpc_global_thread_num(%struct.ident_t* @[[GLOB1:[0-9]+]])
+// CHECK-NEXT:call void @__kmpc_serialized_parallel(%struct.ident_t* @[[GLOB1]], i32 [[TMP0]])
+// CHECK-NEXT:store i32 [[TMP0]], i32* [[DOTTHREADID_TEMP_]], align 4, !tbaa [[TBAA3:![0-9]+]]
+// CHECK-NEXT:call void @.omp_outlined.(i32* [[DOTTHREADID_TEMP_]], i32* [[DOTBOUND_ZERO_ADDR]])

[PATCH] D107649: [OpenMP]Fix PR51349: Remove AlwaysInline for if regions.

2021-08-06 Thread Mike Rice via Phabricator via cfe-commits 
mikerice added a comment.

Works for the test we have.  What about a lit test for this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107649

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

[PATCH] D107649: [OpenMP]Fix PR51349: Remove AlwaysInline for if regions.

2021-08-06 Thread Joseph Huber via Phabricator via cfe-commits 
jhuber6 created this revision.
jhuber6 added reviewers: jdoerfert, mikerice.
Herald added subscribers: guansong, yaxunl.
jhuber6 requested review of this revision.
Herald added subscribers: cfe-commits, sstefan1.
Herald added a project: clang.

After D94315  we add the `NoInline` attribute 
to the outlined function to handle
data environments in the OpenMP if clause. This conflicted with the 
`AlwaysInline`
attribute added to the outlined function. for better performance in D106799 
.
The data environments should ideally not require NoInline, but for now this
fixes PR51349.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D107649

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp


Index: clang/lib/CodeGen/CGOpenMPRuntime.cpp
===
--- clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -2120,11 +2120,12 @@
 OutlinedFnArgs.append(CapturedVars.begin(), CapturedVars.end());
 
 // Ensure we do not inline the function. This is trivially true for the 
ones
-// passed to __kmpc_fork_call but the ones calles in serialized regions
+// passed to __kmpc_fork_call but the ones called in serialized regions
 // could be inlined. This is not a perfect but it is closer to the 
invariant
 // we want, namely, every data environment starts with a new function.
 // TODO: We should pass the if condition to the runtime function and do the
 //   handling there. Much cleaner code.
+OutlinedFn->removeFnAttr(llvm::Attribute::AlwaysInline);
 OutlinedFn->addFnAttr(llvm::Attribute::NoInline);
 RT.emitOutlinedFunctionCall(CGF, Loc, OutlinedFn, OutlinedFnArgs);
 


Index: clang/lib/CodeGen/CGOpenMPRuntime.cpp
===
--- clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -2120,11 +2120,12 @@
 OutlinedFnArgs.append(CapturedVars.begin(), CapturedVars.end());
 
 // Ensure we do not inline the function. This is trivially true for the ones
-// passed to __kmpc_fork_call but the ones calles in serialized regions
+// passed to __kmpc_fork_call but the ones called in serialized regions
 // could be inlined. This is not a perfect but it is closer to the invariant
 // we want, namely, every data environment starts with a new function.
 // TODO: We should pass the if condition to the runtime function and do the
 //   handling there. Much cleaner code.
+OutlinedFn->removeFnAttr(llvm::Attribute::AlwaysInline);
 OutlinedFn->addFnAttr(llvm::Attribute::NoInline);
 RT.emitOutlinedFunctionCall(CGF, Loc, OutlinedFn, OutlinedFnArgs);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits