From c0b2a943df3ab2babeda19b68ee076f10a76f481 Mon Sep 17 00:00:00 2001 From: Slava Zakharin Date: Tue, 18 Mar 2025 21:27:04 -0700 Subject: [PATCH 1/2] [flang] Fixed repacking for TARGET and INTENT(OUT) TARGET dummy arrays can be accessed indirectly, so it is unsafe to repack them. INTENT(OUT) dummy arrays that require finalization on entry to their subroutine must be copied-in by `fir.pack_arrays`. In addition, based on my testing results, I think it will be useful to document that `LOC` and `IS_CONTIGUOUS` will have different values for the repacked arrays. I still need to decide where to document this, so just added a note in the design doc for the time being. --- flang/docs/ArrayRepacking.md | 3 ++ flang/lib/Lower/ConvertVariable.cpp | 31 ++++++++++++++----- .../Lower/repack-arrays-finalized-dummy.f90 | 31 +++++++++++++++++++ flang/test/Lower/repack-arrays-target.f90 | 10 ++++++ 4 files changed, 68 insertions(+), 7 deletions(-) create mode 100644 flang/test/Lower/repack-arrays-finalized-dummy.f90 create mode 100644 flang/test/Lower/repack-arrays-target.f90 diff --git a/flang/docs/ArrayRepacking.md b/flang/docs/ArrayRepacking.md index 38485fd1e4d18..f22e26ce49738 100755 --- a/flang/docs/ArrayRepacking.md +++ b/flang/docs/ArrayRepacking.md @@ -145,6 +145,7 @@ So it does not seem practical/reasonable to enable the array repacking by defaul 3. Provide consistent behavior of the temporary arrays with relation to `-fstack-arrays` (that forces all temporary arrays to be allocated on the stack). 4. Produce correct debug information to substitute the original array with the copy array when accessing values in the debugger. 5. Document potential correctness issues that array repacking may cause in multithreaded/offload execution. +6. Document the expected changes of the programs behavior, such as applying `LOC` and `IS_CONTIGUOUS` intrinsic functions to the repacked arrays (one cannot expect the same results as if these intrinsics were applied to the original arrays). ## Proposed design @@ -346,6 +347,8 @@ The copy creation is also restricted for `ASYNCHRONOUS` and `VOLATILE` arguments It does not make sense to generate the new operations for `CONTIGUOUS` arguments and for arguments with statically known element size that exceeds the `max-element-size` threshold. +The `fir.pack_array`'s copy-in action cannot be skipped for `INTENT(OUT)` dummy argument of a derived type that requires finalization on entry to the subprogram, as long as the finalization subroutines may access the value of the dummy argument. In this case `fir.pack_array` operation cannot have `no_copy` attribute, so that it creates a contiguous temporary matching the value of the original array, and then the temporary is finalized before execution of the subprogram's body begins. + #### Optional behavior In case of the `whole` continuity mode or with 1-D array, Flang can propagate this information to `hlfir.declare` - this may improve optimizations down the road. This can be done iff the repacking has no dynamic constraints and/or heuristics. For example: diff --git a/flang/lib/Lower/ConvertVariable.cpp b/flang/lib/Lower/ConvertVariable.cpp index 52e3578ae21f0..f2bb5728ac353 100644 --- a/flang/lib/Lower/ConvertVariable.cpp +++ b/flang/lib/Lower/ConvertVariable.cpp @@ -916,10 +916,7 @@ needDeallocationOrFinalization(const Fortran::lower::pft::Variable &var) { /// point 7. /// Must be nonpointer, nonallocatable, INTENT (OUT) dummy argument. static bool -needDummyIntentoutFinalization(const Fortran::lower::pft::Variable &var) { - if (!var.hasSymbol()) - return false; - const Fortran::semantics::Symbol &sym = var.getSymbol(); +needDummyIntentoutFinalization(const Fortran::semantics::Symbol &sym) { if (!Fortran::semantics::IsDummy(sym) || !Fortran::semantics::IsIntentOut(sym) || Fortran::semantics::IsAllocatable(sym) || @@ -938,6 +935,16 @@ needDummyIntentoutFinalization(const Fortran::lower::pft::Variable &var) { return hasFinalization(sym) || hasAllocatableDirectComponent(sym); } +/// Check whether a variable needs the be finalized according to clause 7.5.6.3 +/// point 7. +/// Must be nonpointer, nonallocatable, INTENT (OUT) dummy argument. +static bool +needDummyIntentoutFinalization(const Fortran::lower::pft::Variable &var) { + if (!var.hasSymbol()) + return false; + return needDummyIntentoutFinalization(var.getSymbol()); +} + /// Call default initialization runtime routine to initialize \p var. static void finalizeAtRuntime(Fortran::lower::AbstractConverter &converter, const Fortran::lower::pft::Variable &var, @@ -1011,10 +1018,16 @@ static void deallocateIntentOut(Fortran::lower::AbstractConverter &converter, static bool needsRepack(Fortran::lower::AbstractConverter &converter, const Fortran::semantics::Symbol &sym) { + const auto &attrs = sym.attrs(); if (!converter.getLoweringOptions().getRepackArrays() || !converter.isRegisteredDummySymbol(sym) || !Fortran::semantics::IsAssumedShape(sym) || - Fortran::evaluate::IsSimplyContiguous(sym, converter.getFoldingContext())) + Fortran::evaluate::IsSimplyContiguous(sym, + converter.getFoldingContext()) || + // TARGET dummy may be accessed indirectly, so it is unsafe + // to repack it. Some compilers provide options to override + // this. + attrs.test(Fortran::semantics::Attr::TARGET)) return false; return true; @@ -2613,8 +2626,12 @@ Fortran::lower::genPackArray(Fortran::lower::AbstractConverter &converter, bool stackAlloc = opts.getStackArrays(); // 1D arrays must always use 'whole' mode. bool isInnermostMode = !opts.getRepackArraysWhole() && sym.Rank() > 1; - // Avoid copy-in for 'intent(out)' variables. - bool noCopy = Fortran::semantics::IsIntentOut(sym); + // Avoid copy-in for 'intent(out)' variable, unless this is a dummy + // argument with INTENT(OUT) that needs finalization on entry + // to the subprogram. The finalization routine may read the initial + // value of the array. + bool noCopy = Fortran::semantics::IsIntentOut(sym) && + !needDummyIntentoutFinalization(sym); auto boxType = mlir::cast(fir::getBase(exv).getType()); mlir::Type elementType = boxType.unwrapInnerType(); llvm::SmallVector elidedLenParams = diff --git a/flang/test/Lower/repack-arrays-finalized-dummy.f90 b/flang/test/Lower/repack-arrays-finalized-dummy.f90 new file mode 100644 index 0000000000000..0c0a914d08056 --- /dev/null +++ b/flang/test/Lower/repack-arrays-finalized-dummy.f90 @@ -0,0 +1,31 @@ +! RUN: bbc -emit-hlfir -frepack-arrays %s -o - -I nowhere | FileCheck --check-prefixes=CHECK %s + +! Check that the original array is copied on entry to the subroutine +! before it is being finalized, otherwise the finalization routine +! may read the uninitialized temporary. +! Verify that fir.pack_array does not have no_copy attribute. + +module m + type t + contains + final :: my_final + end type t + interface + subroutine my_final(x) + type(t) :: x(:) + end subroutine my_final + end interface +contains +! CHECK-LABEL: func.func @_QMmPtest( +! CHECK-SAME: %[[VAL_0:[0-9]+|[a-zA-Z$._-][a-zA-Z0-9$._-]*]]: !fir.class>> {fir.bindc_name = "x"}) { + subroutine test(x) + class(t), intent(out) :: x(:) +! CHECK: %[[VAL_2:.*]] = fir.pack_array %[[VAL_0]] heap whole : (!fir.class>>) -> !fir.class>> +! CHECK: %[[VAL_3:.*]]:2 = hlfir.declare %[[VAL_2]] +! CHECK: %[[VAL_4:.*]] = fir.convert %[[VAL_3]]#1 +! CHECK: fir.call @_FortranADestroy(%[[VAL_4]]) fastmath : (!fir.box) -> () +! CHECK: %[[VAL_7:.*]] = fir.convert %[[VAL_3]]#1 +! CHECK: fir.call @_FortranAInitialize(%[[VAL_7]] +! CHECK: fir.unpack_array %[[VAL_2]] to %[[VAL_0]] heap : !fir.class>> + end subroutine test +end module m diff --git a/flang/test/Lower/repack-arrays-target.f90 b/flang/test/Lower/repack-arrays-target.f90 new file mode 100644 index 0000000000000..9254da34d2609 --- /dev/null +++ b/flang/test/Lower/repack-arrays-target.f90 @@ -0,0 +1,10 @@ +! RUN: bbc -emit-hlfir -frepack-arrays %s -o - -I nowhere | FileCheck --check-prefixes=CHECK %s + +! Check that there is no repacking for TARGET dummy argument. + +! CHECK-LABEL: func.func @_QPtest( +! CHECK-NOT: fir.pack_array +! CHECK-NOT: fir.unpack_array +subroutine test(x) + integer, target :: x(:) +end subroutine test From b0db164fa297a52beae66083d0589027a4d81b92 Mon Sep 17 00:00:00 2001 From: Slava Zakharin Date: Wed, 19 Mar 2025 08:33:37 -0700 Subject: [PATCH 2/2] Check for VOLATILE and ASYNCHRONOUS. --- flang/lib/Lower/ConvertVariable.cpp | 9 ++++++++- flang/test/Lower/repack-arrays-asynchronous.f90 | 10 ++++++++++ flang/test/Lower/repack-arrays-target.f90 | 2 +- 3 files changed, 19 insertions(+), 2 deletions(-) create mode 100644 flang/test/Lower/repack-arrays-asynchronous.f90 diff --git a/flang/lib/Lower/ConvertVariable.cpp b/flang/lib/Lower/ConvertVariable.cpp index f2bb5728ac353..0b22b743edee9 100644 --- a/flang/lib/Lower/ConvertVariable.cpp +++ b/flang/lib/Lower/ConvertVariable.cpp @@ -1016,6 +1016,10 @@ static void deallocateIntentOut(Fortran::lower::AbstractConverter &converter, } } +/// Return true iff the given symbol represents a dummy array +/// that needs to be repacked when -frepack-arrays is set. +/// In general, the repacking is done for assumed-shape +/// dummy arguments, but there are limitations. static bool needsRepack(Fortran::lower::AbstractConverter &converter, const Fortran::semantics::Symbol &sym) { const auto &attrs = sym.attrs(); @@ -1027,7 +1031,10 @@ static bool needsRepack(Fortran::lower::AbstractConverter &converter, // TARGET dummy may be accessed indirectly, so it is unsafe // to repack it. Some compilers provide options to override // this. - attrs.test(Fortran::semantics::Attr::TARGET)) + // Repacking of VOLATILE and ASYNCHRONOUS is also unsafe. + attrs.HasAny({Fortran::semantics::Attr::ASYNCHRONOUS, + Fortran::semantics::Attr::TARGET, + Fortran::semantics::Attr::VOLATILE})) return false; return true; diff --git a/flang/test/Lower/repack-arrays-asynchronous.f90 b/flang/test/Lower/repack-arrays-asynchronous.f90 new file mode 100644 index 0000000000000..7e106f33fa6d5 --- /dev/null +++ b/flang/test/Lower/repack-arrays-asynchronous.f90 @@ -0,0 +1,10 @@ +! RUN: bbc -emit-hlfir -frepack-arrays %s -o - | FileCheck --check-prefixes=CHECK %s + +! Check that there is no repacking for ASYNCHRONOUS dummy argument. + +! CHECK-LABEL: func.func @_QPtest( +! CHECK-NOT: fir.pack_array +! CHECK-NOT: fir.unpack_array +subroutine test(x) + integer, asynchronous :: x(:) +end subroutine test diff --git a/flang/test/Lower/repack-arrays-target.f90 b/flang/test/Lower/repack-arrays-target.f90 index 9254da34d2609..d0ad65373ac75 100644 --- a/flang/test/Lower/repack-arrays-target.f90 +++ b/flang/test/Lower/repack-arrays-target.f90 @@ -1,4 +1,4 @@ -! RUN: bbc -emit-hlfir -frepack-arrays %s -o - -I nowhere | FileCheck --check-prefixes=CHECK %s +! RUN: bbc -emit-hlfir -frepack-arrays %s -o - | FileCheck --check-prefixes=CHECK %s ! Check that there is no repacking for TARGET dummy argument.