From aef8a2c483149d4ad2546843350002f11eb48a20 Mon Sep 17 00:00:00 2001 From: Krish Gupta Date: Tue, 10 Feb 2026 17:01:24 +0530 Subject: [PATCH] [Flang][OpenMP] Fix crash with character types in declare_reduction (#178038) Fixes #177501 This PR fixes a compilation crash when using character types in OpenMP REDUCTION clauses with declare_reduction directives. The problem was that character types weren't being handled properly during OpenMP lowering. Specifically: - Missing character length parameters in hlfir.declare operations - Incorrect type wrapping for by-ref reductions - Missing special case handling for boxed/unboxed character types The fix ensures character types are treated similarly to derived types throughout the reduction pipeline, since fir::isa_trivial() excludes them. Added a regression test to verify the fix works for both allocatable and non-allocatable character reductions. image --- flang/lib/Lower/OpenMP/ClauseProcessor.cpp | 36 ++++++++++++++++--- flang/lib/Lower/OpenMP/OpenMP.cpp | 19 +++++++--- .../Lower/Support/PrivateReductionUtils.cpp | 35 +++++++++++++----- .../lib/Lower/Support/ReductionProcessor.cpp | 5 +++ .../reduction-character-dynamic-length.f90 | 11 ++++++ ...eclare-reduction-character-allocatable.f90 | 33 +++++++++++++++++ 6 files changed, 121 insertions(+), 18 deletions(-) create mode 100644 flang/test/Lower/OpenMP/Todo/reduction-character-dynamic-length.f90 create mode 100644 flang/test/Lower/OpenMP/declare-reduction-character-allocatable.f90 diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp index b1973a3b8bf0..9f9dd436dc6a 100644 --- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp +++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp @@ -394,16 +394,42 @@ bool ClauseProcessor::processInitializer( for (const Object &object : std::get(inst.t)) { - mlir::Value addr = builder.createTemporary(loc, ompOrig.getType()); - fir::StoreOp::create(builder, loc, ompOrig, addr); + mlir::Value addr; + mlir::Type ompOrigType = ompOrig.getType(); + // Check for unsupported dynamic-length character reductions + mlir::Type unwrappedType = fir::unwrapRefType(ompOrigType); + if (mlir::isa(unwrappedType)) { + TODO(loc, "OpenMP reduction allocation for dynamic length character"); + } + if (auto charTy = mlir::dyn_cast(unwrappedType)) { + if (!charTy.hasConstantLen()) { + TODO(loc, + "OpenMP reduction allocation for dynamic length character"); + } + } + // If ompOrig is already a reference, we can use it directly + if (fir::isa_ref_type(ompOrigType)) { + addr = ompOrig; + } else { + addr = builder.createTemporary(loc, ompOrigType); + fir::StoreOp::create(builder, loc, ompOrig, addr); + } fir::FortranVariableFlagsEnum extraFlags = {}; fir::FortranVariableFlagsAttr attributes = Fortran::lower::translateSymbolAttributes( builder.getContext(), *object.sym(), extraFlags); std::string name = object.sym()->name().ToString(); - auto declareOp = - hlfir::DeclareOp::create(builder, loc, addr, name, nullptr, {}, - nullptr, nullptr, 0, attributes); + // Get length parameters for types that need them (e.g., characters). + // Note: DeclareOp requires exactly one type parameter for non-boxed + // characters, unlike EmboxOp which doesn't allow them for constant-len. + llvm::SmallVector typeParams; + if (hlfir::isFortranEntity(addr)) { + hlfir::genLengthParameters(loc, builder, hlfir::Entity{addr}, + typeParams); + } + auto declareOp = hlfir::DeclareOp::create(builder, loc, addr, name, + nullptr, typeParams, nullptr, + nullptr, 0, attributes); if (name == "omp_priv") ompPrivVar = declareOp.getResult(0); symMap.addVariableDefinition(*object.sym(), declareOp); diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp index 1d5b8b5ac436..f7be823335bd 100644 --- a/flang/lib/Lower/OpenMP/OpenMP.cpp +++ b/flang/lib/Lower/OpenMP/OpenMP.cpp @@ -3765,9 +3765,15 @@ static ReductionProcessor::GenCombinerCBTy processReductionCombiner( fir::FortranVariableFlagsAttr attributes = Fortran::lower::translateSymbolAttributes(builder.getContext(), *object.sym(), extraFlags); + // For character types, we need to provide the length parameter + llvm::SmallVector typeParams; + if (hlfir::isFortranEntity(addr)) { + hlfir::genLengthParameters(loc, builder, hlfir::Entity{addr}, + typeParams); + } auto declareOp = - hlfir::DeclareOp::create(builder, loc, addr, name, nullptr, {}, - nullptr, nullptr, 0, attributes); + hlfir::DeclareOp::create(builder, loc, addr, name, nullptr, + typeParams, nullptr, nullptr, 0, attributes); if (name == "omp_out") ompOutVar = declareOp.getResult(0); symTable.addVariableDefinition(*object.sym(), declareOp); @@ -3787,10 +3793,13 @@ static ReductionProcessor::GenCombinerCBTy processReductionCombiner( loc, converter, evalExpr, symTable, stmtCtx)); // Optional load may be generated if we get a reference to the // reduction type. - if (auto refType = - llvm::dyn_cast(exprResult.getType())) - if (lhs.getType() == refType.getElementType()) + if (auto refType = llvm::dyn_cast( + exprResult.getType())) { + mlir::Type expectedType = + isByRef ? fir::unwrapRefType(lhs.getType()) : lhs.getType(); + if (expectedType == refType.getElementType()) exprResult = fir::LoadOp::create(builder, loc, exprResult); + } return exprResult; }}, evalExpr.u); diff --git a/flang/lib/Lower/Support/PrivateReductionUtils.cpp b/flang/lib/Lower/Support/PrivateReductionUtils.cpp index c6c428860bca..d1a965d288ca 100644 --- a/flang/lib/Lower/Support/PrivateReductionUtils.cpp +++ b/flang/lib/Lower/Support/PrivateReductionUtils.cpp @@ -89,12 +89,14 @@ static void createCleanupRegion(Fortran::lower::AbstractConverter &converter, mlir::Value arg = builder.loadIfRef(loc, block->getArgument(0)); assert(mlir::isa(arg.getType())); - // Deallocate box - // The FIR type system doesn't nesecarrily know that this is a mutable box - // if we allocated the thread local array on the heap to avoid looped stack - // allocations. + // Extract address from the box for deallocation. + // The FIR type system doesn't necessarily know that this is a mutable + // box if we allocated the thread local array on the heap to avoid looped + // stack allocations. mlir::Value addr = hlfir::genVariableRawAddress(loc, builder, hlfir::Entity{arg}); + + // Deallocate if allocated mlir::Value isAllocated = builder.genIsNotNullAddr(loc, addr); fir::IfOp ifOp = fir::IfOp::create(builder, loc, isAllocated, /*withElseRegion=*/false); @@ -112,6 +114,10 @@ static void createCleanupRegion(Fortran::lower::AbstractConverter &converter, return; } + // Handle !fir.boxchar (passed by VALUE for runtime-length characters). + // Note: This is distinct from !fir.box> which is handled above. + // BoxChar is a special tuple type (addr, len) used when character length + // is only known at runtime. if (auto boxCharTy = mlir::dyn_cast(argType)) { auto [addr, len] = fir::factory::CharacterExprHelper{builder, loc}.createUnboxChar( @@ -620,7 +626,9 @@ void PopulateInitAndCleanupRegionsHelper::populateByRefInitAndCleanupRegions() { assert(sym && "Symbol information is required to privatize derived types"); assert(!scalarInitValue && "ScalarInitvalue is unused for privatization"); } - if (hlfir::Entity{moldArg}.isAssumedRank()) + // Only check for assumed rank if moldArg is a valid Fortran entity. + // Boxed types (like allocatable characters) may not be valid entities yet. + if (hlfir::isFortranEntity(moldArg) && hlfir::Entity{moldArg}.isAssumedRank()) TODO(loc, "Privatization of assumed rank variable"); mlir::Type valTy = fir::unwrapRefType(argType); @@ -649,8 +657,10 @@ void PopulateInitAndCleanupRegionsHelper::populateByRefInitAndCleanupRegions() { bool isChar = fir::isa_char(innerTy); if (fir::isa_trivial(innerTy) || isDerived || isChar) { // boxed non-sequence value e.g. !fir.box> - if ((isDerived || isChar) && (isReduction(kind) || scalarInitValue)) - TODO(loc, "Reduction of an unsupported boxed type"); + // Character types in reductions are supported, but derived types are not + // yet. + if (isDerived && (isReduction(kind) || scalarInitValue)) + TODO(loc, "Reduction of an unsupported boxed derived type"); initAndCleanupBoxedScalar(boxTy, needsInitialization); return; } @@ -663,10 +673,19 @@ void PopulateInitAndCleanupRegionsHelper::populateByRefInitAndCleanupRegions() { } // Unboxed types: - if (auto boxCharTy = mlir::dyn_cast(argType)) { + if (auto boxCharTy = mlir::dyn_cast(valTy)) { initAndCleanupBoxchar(boxCharTy); return; } + // Handle unboxed character types (e.g., !fir.char<1,1>). + // For fixed-length character types, we just need to initialize the value. + if (fir::isa_char(valTy)) { + builder.setInsertionPointToEnd(initBlock); + if (scalarInitValue) + builder.createStoreWithConvert(loc, scalarInitValue, allocatedPrivVarArg); + createYield(allocatedPrivVarArg); + return; + } if (fir::isa_derived(valType)) { initAndCleanupUnboxedDerivedType(needsInitialization); return; diff --git a/flang/lib/Lower/Support/ReductionProcessor.cpp b/flang/lib/Lower/Support/ReductionProcessor.cpp index 0e01268dd74f..e0cba4c51225 100644 --- a/flang/lib/Lower/Support/ReductionProcessor.cpp +++ b/flang/lib/Lower/Support/ReductionProcessor.cpp @@ -582,6 +582,11 @@ DeclareRedType ReductionProcessor::createDeclareReductionHelper( if (isByRef) { boxedTy = fir::unwrapPassByRefType(valTy); boxedTyAttr = mlir::TypeAttr::get(boxedTy); + // For character types that are not already references, we need to wrap + // them in a reference type for by-ref reductions. + if (fir::isa_char(valTy) && !fir::isa_ref_type(type)) { + type = fir::ReferenceType::get(valTy); + } } else type = valTy; diff --git a/flang/test/Lower/OpenMP/Todo/reduction-character-dynamic-length.f90 b/flang/test/Lower/OpenMP/Todo/reduction-character-dynamic-length.f90 new file mode 100644 index 000000000000..b18c6f6c6fb7 --- /dev/null +++ b/flang/test/Lower/OpenMP/Todo/reduction-character-dynamic-length.f90 @@ -0,0 +1,11 @@ +! RUN: %not_todo_cmd bbc -emit-hlfir -fopenmp -o - %s 2>&1 | FileCheck %s +! RUN: %not_todo_cmd %flang_fc1 -emit-hlfir -fopenmp -o - %s 2>&1 | FileCheck %s + +! CHECK: not yet implemented: OpenMP reduction allocation for dynamic length character +subroutine test_dynamic_length(n) + integer, intent(in) :: n + character(len=n) :: var + + !$omp declare reduction (char_max_dyn:character(len=n):omp_out=max(omp_out,omp_in)) & + !$omp initializer(omp_priv='a') +end subroutine test_dynamic_length diff --git a/flang/test/Lower/OpenMP/declare-reduction-character-allocatable.f90 b/flang/test/Lower/OpenMP/declare-reduction-character-allocatable.f90 new file mode 100644 index 000000000000..daa0d4106385 --- /dev/null +++ b/flang/test/Lower/OpenMP/declare-reduction-character-allocatable.f90 @@ -0,0 +1,33 @@ +! Test OpenMP declare reduction with character type and allocatable variable. +! This test ensures that Flang doesn't crash when processing user-defined +! reductions with character types on allocatable variables. +! See issue: https://github.com/llvm/llvm-project/issues/177501 + +! RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s + +! Test basic character reduction with allocatable variable (constant length) +program test_character_reduction + character(len=1), allocatable :: k1 + + !$omp declare reduction (char_max:character(len=1):omp_out=max(omp_out,omp_in)) & + !$omp initializer(omp_priv='a') + + !$omp parallel sections reduction(char_max:k1) + k1 = max(k1, 'z') + !$omp end parallel sections + +end program test_character_reduction + +! Verify the declare_reduction is generated with reference type for character +! CHECK-LABEL: omp.declare_reduction @char_max : !fir.ref> +! CHECK: init { +! CHECK: omp.yield + +! Verify the combiner region works +! CHECK: combiner +! CHECK: hlfir.declare +! CHECK: omp.yield + +! Verify the reduction is used in the parallel sections +! CHECK: omp.parallel +! CHECK: omp.sections reduction(byref @char_max