[PATCH] D150461: [OpenMP] Naturally align internal global variables in the OpenMPIRBuilder

[Joseph Huber via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGdd02984519ab: [OpenMP] Naturally align internal global 
variables in the OpenMPIRBuilder (authored by jhuber6).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150461

Files:
  clang/test/OpenMP/distribute_parallel_for_reduction_task_codegen.cpp
  clang/test/OpenMP/for_reduction_task_codegen.cpp
  clang/test/OpenMP/parallel_for_reduction_task_codegen.cpp
  clang/test/OpenMP/parallel_master_reduction_task_codegen.cpp
  clang/test/OpenMP/parallel_reduction_task_codegen.cpp
  clang/test/OpenMP/parallel_sections_reduction_task_codegen.cpp
  clang/test/OpenMP/reduction_implicit_map.cpp
  clang/test/OpenMP/sections_reduction_task_codegen.cpp
  clang/test/OpenMP/target_parallel_for_reduction_task_codegen.cpp
  clang/test/OpenMP/target_parallel_reduction_task_codegen.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_reduction_task_codegen.cpp
  clang/test/OpenMP/taskloop_reduction_codegen.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_reduction_task_codegen.cpp
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -4225,10 +4225,12 @@
 // variable for possibly changing that to internal or private, or maybe
 // create different versions of the function for different OMP internal
 // variables.
-Elem.second = new GlobalVariable(
+auto *GV = new GlobalVariable(
 M, Ty, /*IsConstant=*/false, GlobalValue::CommonLinkage,
 Constant::getNullValue(Ty), Elem.first(),
 /*InsertBefore=*/nullptr, GlobalValue::NotThreadLocal, AddressSpace);
+GV->setAlignment(M.getDataLayout().getABITypeAlign(Ty));
+Elem.second = GV;
   }
 
   return cast(&*Elem.second);
Index: clang/test/OpenMP/teams_distribute_parallel_for_reduction_task_codegen.cpp
===
--- clang/test/OpenMP/teams_distribute_parallel_for_reduction_task_codegen.cpp
+++ clang/test/OpenMP/teams_distribute_parallel_for_reduction_task_codegen.cpp
@@ -348,7 +348,7 @@
 // CHECK1-NEXT:store ptr [[TMP0]], ptr [[DOTADDR]], align 8
 // CHECK1-NEXT:store ptr [[TMP1]], ptr [[DOTADDR1]], align 8
 // CHECK1-NEXT:[[TMP2:%.*]] = load ptr, ptr [[DOTADDR]], align 8
-// CHECK1-NEXT:[[TMP3:%.*]] = call ptr @llvm.threadlocal.address.p0(ptr @{{reduction_size[.].+[.]}})
+// CHECK1-NEXT:[[TMP3:%.*]] = call align 8 ptr @llvm.threadlocal.address.p0(ptr align 8 @{{reduction_size[.].+[.]}})
 // CHECK1-NEXT:[[TMP4:%.*]] = load i64, ptr [[TMP3]], align 8
 // CHECK1-NEXT:[[TMP5:%.*]] = getelementptr i8, ptr [[TMP2]], i64 [[TMP4]]
 // CHECK1-NEXT:[[OMP_ARRAYINIT_ISEMPTY:%.*]] = icmp eq ptr [[TMP2]], [[TMP5]]
@@ -370,7 +370,7 @@
 // CHECK1-NEXT:[[DOTADDR1:%.*]] = alloca ptr, align 8
 // CHECK1-NEXT:store ptr [[TMP0]], ptr [[DOTADDR]], align 8
 // CHECK1-NEXT:store ptr [[TMP1]], ptr [[DOTADDR1]], align 8
-// CHECK1-NEXT:[[TMP2:%.*]] = call ptr @llvm.threadlocal.address.p0(ptr @{{reduction_size[.].+[.]}})
+// CHECK1-NEXT:[[TMP2:%.*]] = call align 8 ptr @llvm.threadlocal.address.p0(ptr align 8 @{{reduction_size[.].+[.]}})
 // CHECK1-NEXT:[[TMP3:%.*]] = load i64, ptr [[TMP2]], align 8
 // CHECK1-NEXT:[[TMP4:%.*]] = load ptr, ptr [[DOTADDR]], align 8
 // CHECK1-NEXT:[[TMP5:%.*]] = load ptr, ptr [[DOTADDR1]], align 8
@@ -712,7 +712,7 @@
 // CHECK1-NEXT:store ptr [[TMP0]], ptr [[DOTADDR]], align 8
 // CHECK1-NEXT:store ptr [[TMP1]], ptr [[DOTADDR1]], align 8
 // CHECK1-NEXT:[[TMP2:%.*]] = load ptr, ptr [[DOTADDR]], align 8
-// CHECK1-NEXT:[[TMP3:%.*]] = call ptr @llvm.threadlocal.address.p0(ptr @{{reduction_size[.].+[.]}})
+// CHECK1-NEXT:[[TMP3:%.*]] = call align 8 ptr @llvm.threadlocal.address.p0(ptr align 8 @{{reduction_size[.].+[.]}})
 // CHECK1-NEXT:[[TMP4:%.*]] = load i64, ptr [[TMP3]], align 8
 // CHECK1-NEXT:[[TMP5:%.*]] = getelementptr i8, ptr [[TMP2]], i64 [[TMP4]]
 // CHECK1-NEXT:[[OMP_ARRAYINIT_ISEMPTY:%.*]] = icmp eq ptr [[TMP2]], [[TMP5]]
@@ -734,7 +734,7 @@
 // CHECK1-NEXT:[[DOTADDR1:%.*]] = alloca ptr, align 8
 // CHECK1-NEXT:store ptr [[TMP0]], ptr [[DOTADDR]], align 8
 // CHECK1-NEXT:store ptr [[TMP1]], ptr [[DOTADDR1]], align 8
-// CHECK1-NEXT:[[TMP2:%.*]] = call ptr @llvm.threadlocal.address.p0(ptr @{{reduction_size[.].+[.]}})
+// CHECK1-NEXT:[[TMP2:%.*]] = call align 8 ptr @llvm.threadlocal.address.p0(ptr align 8 @{{reduction_size[.].+[.]}})
 // CHECK1-NEXT:[[TMP3:%.*]] = load i64, ptr [[TMP2]], align 8
 // CHECK1-NEXT:[[TMP4:%.*]] = load ptr, ptr [[DOTADDR]], align 8
 // CHECK1-NEXT:[[TMP5:%.*]] = load ptr, 

[PATCH] D150461: [OpenMP] Naturally align internal global variables in the OpenMPIRBuilder

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet added a comment.

In D150461#4338499 , @jhuber6 wrote:

> In D150461#4338498 , @gchatelet 
> wrote:
>
>> quick question, did you try to build the openmp runtime as well to check if 
>> the tests still pass?
>
> I'm an OpenMP developer so I always have it built : ). Yes they still pass.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150461

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

[PATCH] D150461: [OpenMP] Naturally align internal global variables in the OpenMPIRBuilder

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added a comment.

In D150461#4338498 , @gchatelet wrote:

> quick question, did you try to build the openmp runtime as well to check if 
> the tests still pass?

I'm an OpenMP developer so I always have it built : ). Yes they still pass.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150461

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

[PATCH] D150461: [OpenMP] Naturally align internal global variables in the OpenMPIRBuilder

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet added a comment.

quick question, did you try to build the openmp runtime as well to check if the 
tests still pass?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150461

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

[PATCH] D150461: [OpenMP] Naturally align internal global variables in the OpenMPIRBuilder

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet accepted this revision.
gchatelet added a comment.

You beat me to it :)
Thx for the fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150461

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

[PATCH] D150461: [OpenMP] Naturally align internal global variables in the OpenMPIRBuilder

[Shilei Tian via Phabricator via cfe-commits]

tianshilei1992 accepted this revision.
tianshilei1992 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/D150461/new/

https://reviews.llvm.org/D150461

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

[PATCH] D150461: [OpenMP] Naturally align internal global variables in the OpenMPIRBuilder

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 created this revision.
jhuber6 added reviewers: jdoerfert, tianshilei1992, ABataev, JonChesterfield, 
tstellar, gchatelet.
Herald added subscribers: sunshaoce, guansong, hiraditya, yaxunl.
Herald added a project: All.
jhuber6 requested review of this revision.
Herald added subscribers: llvm-commits, cfe-commits, jplehr, sstefan1.
Herald added projects: clang, LLVM.

We use this helper to make several internal global variables during
codegen. currently we do not specify any alignment which allows the
alignment to be set incorrectly after some changes in how alignment was
handled. This patch explicitly aligns these variables to the natural
alignment as specified by the data layout

Fixes https://github.com/llvm/llvm-project/issues/62668


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150461

Files:
  clang/test/OpenMP/distribute_parallel_for_reduction_task_codegen.cpp
  clang/test/OpenMP/for_reduction_task_codegen.cpp
  clang/test/OpenMP/parallel_for_reduction_task_codegen.cpp
  clang/test/OpenMP/parallel_master_reduction_task_codegen.cpp
  clang/test/OpenMP/parallel_reduction_task_codegen.cpp
  clang/test/OpenMP/parallel_sections_reduction_task_codegen.cpp
  clang/test/OpenMP/reduction_implicit_map.cpp
  clang/test/OpenMP/sections_reduction_task_codegen.cpp
  clang/test/OpenMP/target_parallel_for_reduction_task_codegen.cpp
  clang/test/OpenMP/target_parallel_reduction_task_codegen.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_reduction_task_codegen.cpp
  clang/test/OpenMP/taskloop_reduction_codegen.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_reduction_task_codegen.cpp
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -4225,10 +4225,12 @@
 // variable for possibly changing that to internal or private, or maybe
 // create different versions of the function for different OMP internal
 // variables.
-Elem.second = new GlobalVariable(
+auto *GV = new GlobalVariable(
 M, Ty, /*IsConstant=*/false, GlobalValue::CommonLinkage,
 Constant::getNullValue(Ty), Elem.first(),
 /*InsertBefore=*/nullptr, GlobalValue::NotThreadLocal, AddressSpace);
+GV->setAlignment(M.getDataLayout().getABITypeAlign(Ty));
+Elem.second = GV;
   }
 
   return cast(&*Elem.second);
Index: clang/test/OpenMP/teams_distribute_parallel_for_reduction_task_codegen.cpp
===
--- clang/test/OpenMP/teams_distribute_parallel_for_reduction_task_codegen.cpp
+++ clang/test/OpenMP/teams_distribute_parallel_for_reduction_task_codegen.cpp
@@ -348,7 +348,7 @@
 // CHECK1-NEXT:store ptr [[TMP0]], ptr [[DOTADDR]], align 8
 // CHECK1-NEXT:store ptr [[TMP1]], ptr [[DOTADDR1]], align 8
 // CHECK1-NEXT:[[TMP2:%.*]] = load ptr, ptr [[DOTADDR]], align 8
-// CHECK1-NEXT:[[TMP3:%.*]] = call ptr @llvm.threadlocal.address.p0(ptr @{{reduction_size[.].+[.]}})
+// CHECK1-NEXT:[[TMP3:%.*]] = call align 8 ptr @llvm.threadlocal.address.p0(ptr align 8 @{{reduction_size[.].+[.]}})
 // CHECK1-NEXT:[[TMP4:%.*]] = load i64, ptr [[TMP3]], align 8
 // CHECK1-NEXT:[[TMP5:%.*]] = getelementptr i8, ptr [[TMP2]], i64 [[TMP4]]
 // CHECK1-NEXT:[[OMP_ARRAYINIT_ISEMPTY:%.*]] = icmp eq ptr [[TMP2]], [[TMP5]]
@@ -370,7 +370,7 @@
 // CHECK1-NEXT:[[DOTADDR1:%.*]] = alloca ptr, align 8
 // CHECK1-NEXT:store ptr [[TMP0]], ptr [[DOTADDR]], align 8
 // CHECK1-NEXT:store ptr [[TMP1]], ptr [[DOTADDR1]], align 8
-// CHECK1-NEXT:[[TMP2:%.*]] = call ptr @llvm.threadlocal.address.p0(ptr @{{reduction_size[.].+[.]}})
+// CHECK1-NEXT:[[TMP2:%.*]] = call align 8 ptr @llvm.threadlocal.address.p0(ptr align 8 @{{reduction_size[.].+[.]}})
 // CHECK1-NEXT:[[TMP3:%.*]] = load i64, ptr [[TMP2]], align 8
 // CHECK1-NEXT:[[TMP4:%.*]] = load ptr, ptr [[DOTADDR]], align 8
 // CHECK1-NEXT:[[TMP5:%.*]] = load ptr, ptr [[DOTADDR1]], align 8
@@ -712,7 +712,7 @@
 // CHECK1-NEXT:store ptr [[TMP0]], ptr [[DOTADDR]], align 8
 // CHECK1-NEXT:store ptr [[TMP1]], ptr [[DOTADDR1]], align 8
 // CHECK1-NEXT:[[TMP2:%.*]] = load ptr, ptr [[DOTADDR]], align 8
-// CHECK1-NEXT:[[TMP3:%.*]] = call ptr @llvm.threadlocal.address.p0(ptr @{{reduction_size[.].+[.]}})
+// CHECK1-NEXT:[[TMP3:%.*]] = call align 8 ptr @llvm.threadlocal.address.p0(ptr align 8 @{{reduction_size[.].+[.]}})
 // CHECK1-NEXT:[[TMP4:%.*]] = load i64, ptr [[TMP3]], align 8
 // CHECK1-NEXT:[[TMP5:%.*]] = getelementptr i8, ptr [[TMP2]], i64 [[TMP4]]
 // CHECK1-NEXT:[[OMP_ARRAYINIT_ISEMPTY:%.*]] = icmp eq ptr [[TMP2]], [[TMP5]]
@@ -734,7 +734,7 @@
 // CHECK1-NEXT:[[DOTADDR1:%.*]] = alloca ptr, align 8
 // CHECK1-NEXT:store ptr [[TMP0]], ptr [[DOTADDR]], align 8
 // CHECK1-NEXT:store ptr [[TMP1]], ptr