-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[flang][OpenMP] Initialize allocatable members of derived types #120295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -116,6 +116,31 @@ void DataSharingProcessor::cloneSymbol(const semantics::Symbol *sym) { | |
| *sym, /*skipDefaultInit=*/isFirstPrivate); | ||
| (void)success; | ||
| assert(success && "Privatization failed due to existing binding"); | ||
|
|
||
| // Initialize clone from original object if it has any allocatable member. | ||
| auto needInitClone = [&] { | ||
| if (isFirstPrivate) | ||
| return false; | ||
|
|
||
| SymbolBox sb = symTable.lookupSymbol(sym); | ||
| assert(sb); | ||
| mlir::Value addr = sb.getAddr(); | ||
| assert(addr); | ||
| mlir::Type ty = addr.getType(); | ||
|
|
||
| // Unwrap type | ||
| while (auto eleTy = fir::dyn_cast_ptrOrBoxEleTy(ty)) | ||
| ty = eleTy; | ||
| // For arrays, use its element type | ||
| if (auto seqTy = mlir::dyn_cast<fir::SequenceType>(ty)) | ||
| ty = seqTy.getEleTy(); | ||
| return fir::isRecordWithAllocatableMember(ty); | ||
| }; | ||
|
|
||
| if (needInitClone()) { | ||
| Fortran::lower::initializeCloneAtRuntime(converter, *sym, symTable); | ||
| callsInitClone = true; | ||
| } | ||
| } | ||
|
|
||
| void DataSharingProcessor::copyFirstPrivateSymbol( | ||
|
|
@@ -165,8 +190,8 @@ bool DataSharingProcessor::needBarrier() { | |
| // variables. | ||
| // Emit implicit barrier for linear clause. Maybe on somewhere else. | ||
| for (const semantics::Symbol *sym : allPrivatizedSymbols) { | ||
| if (sym->test(semantics::Symbol::Flag::OmpFirstPrivate) && | ||
| sym->test(semantics::Symbol::Flag::OmpLastPrivate)) | ||
| if (sym->test(semantics::Symbol::Flag::OmpLastPrivate) && | ||
| (sym->test(semantics::Symbol::Flag::OmpFirstPrivate) || callsInitClone)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @luporl Why don't we need this barrier in other cases, e.g. when
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original variable is also used for other similar cases e.g. assumed shape arrays. Maybe for now we should always use the barrier?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will likely affect performance, but it is certainly better than having hard-to-debug race conditions. Later, we could rename
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do I understand correctly that the barrier here isn't strictly necessary because v and v2 are different symbols? I'm trying to add the extra barriers in my re-implementation of this patch in my changes for https://discourse.llvm.org/t/rfc-openmp-supporting-delayed-task-execution-with-firstprivate-variables/83084/7 Thanks for the help!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The barrier there is necessary. It prevents the original As
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! |
||
| return true; | ||
| } | ||
| return false; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| ! Test that derived type allocatable members of private copies are properly | ||
| ! initialized. | ||
| !RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s | ||
|
|
||
| module m1 | ||
| type x | ||
| integer, allocatable :: x1(:) | ||
| end type | ||
|
|
||
| type y | ||
| integer :: y1(10) | ||
| end type | ||
|
|
||
| contains | ||
|
|
||
| !CHECK-LABEL: omp.private {type = private} @_QMm1Ftest_nested | ||
| !CHECK: fir.call @_FortranAInitializeClone | ||
| !CHECK-NEXT: omp.yield | ||
|
|
||
| !CHECK-LABEL: omp.private {type = private} @_QMm1Ftest_array_of_allocs | ||
| !CHECK: fir.call @_FortranAInitializeClone | ||
| !CHECK-NEXT: omp.yield | ||
|
|
||
| !CHECK-LABEL: omp.private {type = firstprivate} @_QMm1Ftest_array | ||
| !CHECK-NOT: fir.call @_FortranAInitializeClone | ||
| !CHECK: omp.yield | ||
|
|
||
| !CHECK-LABEL: omp.private {type = private} @_QMm1Ftest_array | ||
| !CHECK: fir.call @_FortranAInitializeClone | ||
| !CHECK-NEXT: omp.yield | ||
|
|
||
| !CHECK-LABEL: omp.private {type = private} @_QMm1Ftest_scalar | ||
| !CHECK: fir.call @_FortranAInitializeClone | ||
| !CHECK-NEXT: omp.yield | ||
|
|
||
| subroutine test_scalar() | ||
| type(x) :: v | ||
| allocate(v%x1(5)) | ||
|
|
||
| !$omp parallel private(v) | ||
| !$omp end parallel | ||
| end subroutine | ||
|
|
||
| ! Test omp sections lastprivate(v, v2) | ||
| ! - InitializeClone must not be called for v2, that doesn't have an | ||
| ! allocatable member. | ||
| ! - InitializeClone must be called for v, that has an allocatable member. | ||
| ! - To avoid race conditions between InitializeClone and lastprivate, a | ||
| ! barrier must be present after the initializations. | ||
| !CHECK-LABEL: func @_QMm1Ptest_array | ||
| !CHECK: fir.call @_FortranAInitializeClone | ||
| !CHECK-NEXT: omp.barrier | ||
| subroutine test_array() | ||
| type(x) :: v(10) | ||
| type(y) :: v2(10) | ||
| allocate(v(1)%x1(5)) | ||
|
|
||
| !$omp parallel private(v) | ||
| !$omp end parallel | ||
|
|
||
| !$omp parallel | ||
| !$omp sections lastprivate(v2, v) | ||
| !$omp end sections | ||
| !$omp end parallel | ||
|
|
||
| !$omp parallel firstprivate(v) | ||
| !$omp end parallel | ||
| end subroutine | ||
|
|
||
| subroutine test_array_of_allocs() | ||
| type(x), allocatable :: v(:) | ||
| allocate(v(10)) | ||
| allocate(v(1)%x1(5)) | ||
|
|
||
| !$omp parallel private(v) | ||
| !$omp end parallel | ||
| end subroutine | ||
|
|
||
| subroutine test_nested() | ||
| type dt1 | ||
| integer, allocatable :: a(:) | ||
| end type | ||
|
|
||
| type dt2 | ||
| type(dt1) :: d1 | ||
| end type | ||
|
|
||
| type(dt2) :: d2 | ||
| allocate(d2%d1%a(10)) | ||
|
|
||
| !$omp parallel private(d2) | ||
| !$omp end parallel | ||
| end subroutine | ||
| end module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this will also flag polymorphic objects as potentially having allocatable components (the dynamic type extending the static type may have such components and is unknown).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching that.