Skip to content

Commit df1f2da

Browse files
tblahGeorgeARM
tblah
authored and
GeorgeARM
committed
[flang][OpenMP] share global variable initialization code (llvm#138672)
Fixes llvm#108136 In llvm#108136 (the new testcase), flang was missing the length parameter required for the variable length string when boxing the global variable. The code that is initializing global variables for OpenMP did not support types with length parameters. Instead of duplicating this initialization logic in OpenMP, I decided to use the exact same initialization as is used in the base language because this will already be well tested and will be updated for any new types. The difference for OpenMP is that the global variables will be zero initialized instead of left undefined. Previously `Fortran::lower::createGlobalInitialization` was used to share a smaller amount of the logic with the base language lowering. I think this bug has demonstrated that helper was too low level to be helpful, and it was only used in OpenMP so I have made it static inside of ConvertVariable.cpp.
1 parent bba21b3 commit df1f2da

File tree

9 files changed

+81
-88
lines changed

9 files changed

+81
-88
lines changed

‎flang/docs/OpenMPSupport.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,4 +64,4 @@ Note : No distinction is made between the support in Parser/Semantics, MLIR, Low
6464
| target teams distribute parallel loop simd construct | P | device, reduction, dist_schedule and linear clauses are not supported |
6565

6666
## OpenMP 3.1, OpenMP 2.5, OpenMP 1.1
67-
All features except a few corner cases in atomic (complex type, different but compatible types in lhs and rhs), threadprivate (character type) constructs/clauses are supported.
67+
All features except a few corner cases in atomic (complex type, different but compatible types in lhs and rhs) are supported.

‎flang/include/flang/Lower/ConvertVariable.h‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -134,10 +134,11 @@ mlir::Value genInitialDataTarget(Fortran::lower::AbstractConverter &,
134134
const SomeExpr &initialTarget,
135135
bool couldBeInEquivalence = false);
136136

137-
/// Call \p genInit to generate code inside \p global initializer region.
138-
void createGlobalInitialization(
139-
fir::FirOpBuilder &builder, fir::GlobalOp global,
140-
std::function<void(fir::FirOpBuilder &)> genInit);
137+
/// Create the global op and its init if it has one
138+
fir::GlobalOp defineGlobal(Fortran::lower::AbstractConverter &converter,
139+
const Fortran::lower::pft::Variable &var,
140+
llvm::StringRef globalName, mlir::StringAttr linkage,
141+
cuf::DataAttributeAttr dataAttr = {});
141142

142143
/// Generate address \p addr inside an initializer.
143144
fir::ExtendedValue

‎flang/lib/Lower/ConvertVariable.cpp‎

Lines changed: 45 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -145,11 +145,10 @@ static bool isConstant(const Fortran::semantics::Symbol &sym) {
145145
sym.test(Fortran::semantics::Symbol::Flag::ReadOnly);
146146
}
147147

148-
static fir::GlobalOp defineGlobal(Fortran::lower::AbstractConverter &converter,
149-
const Fortran::lower::pft::Variable &var,
150-
llvm::StringRef globalName,
151-
mlir::StringAttr linkage,
152-
cuf::DataAttributeAttr dataAttr = {});
148+
/// Call \p genInit to generate code inside \p global initializer region.
149+
static void
150+
createGlobalInitialization(fir::FirOpBuilder &builder, fir::GlobalOp global,
151+
std::function<void(fir::FirOpBuilder &)> genInit);
153152

154153
static mlir::Location genLocation(Fortran::lower::AbstractConverter &converter,
155154
const Fortran::semantics::Symbol &sym) {
@@ -467,9 +466,9 @@ static bool globalIsInitialized(fir::GlobalOp global) {
467466
}
468467

469468
/// Call \p genInit to generate code inside \p global initializer region.
470-
void Fortran::lower::createGlobalInitialization(
471-
fir::FirOpBuilder &builder, fir::GlobalOp global,
472-
std::function<void(fir::FirOpBuilder &)> genInit) {
469+
static void
470+
createGlobalInitialization(fir::FirOpBuilder &builder, fir::GlobalOp global,
471+
std::function<void(fir::FirOpBuilder &)> genInit) {
473472
mlir::Region &region = global.getRegion();
474473
region.push_back(new mlir::Block);
475474
mlir::Block &block = region.back();
@@ -479,7 +478,7 @@ void Fortran::lower::createGlobalInitialization(
479478
builder.restoreInsertionPoint(insertPt);
480479
}
481480

482-
static unsigned getAllocatorIdx(cuf::DataAttributeAttr dataAttr) {
481+
static unsigned getAllocatorIdxFromDataAttr(cuf::DataAttributeAttr dataAttr) {
483482
if (dataAttr) {
484483
if (dataAttr.getValue() == cuf::DataAttribute::Pinned)
485484
return kPinnedAllocatorPos;
@@ -494,11 +493,10 @@ static unsigned getAllocatorIdx(cuf::DataAttributeAttr dataAttr) {
494493
}
495494

496495
/// Create the global op and its init if it has one
497-
static fir::GlobalOp defineGlobal(Fortran::lower::AbstractConverter &converter,
498-
const Fortran::lower::pft::Variable &var,
499-
llvm::StringRef globalName,
500-
mlir::StringAttr linkage,
501-
cuf::DataAttributeAttr dataAttr) {
496+
fir::GlobalOp Fortran::lower::defineGlobal(
497+
Fortran::lower::AbstractConverter &converter,
498+
const Fortran::lower::pft::Variable &var, llvm::StringRef globalName,
499+
mlir::StringAttr linkage, cuf::DataAttributeAttr dataAttr) {
502500
fir::FirOpBuilder &builder = converter.getFirOpBuilder();
503501
const Fortran::semantics::Symbol &sym = var.getSymbol();
504502
mlir::Location loc = genLocation(converter, sym);
@@ -545,27 +543,25 @@ static fir::GlobalOp defineGlobal(Fortran::lower::AbstractConverter &converter,
545543
sym.detailsIf<Fortran::semantics::ObjectEntityDetails>();
546544
if (details && details->init()) {
547545
auto expr = *details->init();
548-
Fortran::lower::createGlobalInitialization(
549-
builder, global, [&](fir::FirOpBuilder &b) {
550-
mlir::Value box = Fortran::lower::genInitialDataTarget(
551-
converter, loc, symTy, expr);
552-
b.create<fir::HasValueOp>(loc, box);
553-
});
546+
createGlobalInitialization(builder, global, [&](fir::FirOpBuilder &b) {
547+
mlir::Value box =
548+
Fortran::lower::genInitialDataTarget(converter, loc, symTy, expr);
549+
b.create<fir::HasValueOp>(loc, box);
550+
});
554551
} else {
555552
// Create unallocated/disassociated descriptor if no explicit init
556-
Fortran::lower::createGlobalInitialization(
557-
builder, global, [&](fir::FirOpBuilder &b) {
558-
mlir::Value box = fir::factory::createUnallocatedBox(
559-
b, loc, symTy,
560-
/*nonDeferredParams=*/std::nullopt,
561-
/*typeSourceBox=*/{}, getAllocatorIdx(dataAttr));
562-
b.create<fir::HasValueOp>(loc, box);
563-
});
553+
createGlobalInitialization(builder, global, [&](fir::FirOpBuilder &b) {
554+
mlir::Value box = fir::factory::createUnallocatedBox(
555+
b, loc, symTy,
556+
/*nonDeferredParams=*/std::nullopt,
557+
/*typeSourceBox=*/{}, getAllocatorIdxFromDataAttr(dataAttr));
558+
b.create<fir::HasValueOp>(loc, box);
559+
});
564560
}
565561
} else if (const auto *details =
566562
sym.detailsIf<Fortran::semantics::ObjectEntityDetails>()) {
567563
if (details->init()) {
568-
Fortran::lower::createGlobalInitialization(
564+
createGlobalInitialization(
569565
builder, global, [&](fir::FirOpBuilder &builder) {
570566
Fortran::lower::StatementContext stmtCtx(
571567
/*cleanupProhibited=*/true);
@@ -576,7 +572,7 @@ static fir::GlobalOp defineGlobal(Fortran::lower::AbstractConverter &converter,
576572
builder.create<fir::HasValueOp>(loc, castTo);
577573
});
578574
} else if (Fortran::lower::hasDefaultInitialization(sym)) {
579-
Fortran::lower::createGlobalInitialization(
575+
createGlobalInitialization(
580576
builder, global, [&](fir::FirOpBuilder &builder) {
581577
Fortran::lower::StatementContext stmtCtx(
582578
/*cleanupProhibited=*/true);
@@ -591,7 +587,7 @@ static fir::GlobalOp defineGlobal(Fortran::lower::AbstractConverter &converter,
591587
if (details && details->init()) {
592588
auto sym{*details->init()};
593589
if (sym) // Has a procedure target.
594-
Fortran::lower::createGlobalInitialization(
590+
createGlobalInitialization(
595591
builder, global, [&](fir::FirOpBuilder &b) {
596592
Fortran::lower::StatementContext stmtCtx(
597593
/*cleanupProhibited=*/true);
@@ -601,19 +597,17 @@ static fir::GlobalOp defineGlobal(Fortran::lower::AbstractConverter &converter,
601597
b.create<fir::HasValueOp>(loc, castTo);
602598
});
603599
else { // Has NULL() target.
604-
Fortran::lower::createGlobalInitialization(
605-
builder, global, [&](fir::FirOpBuilder &b) {
606-
auto box{fir::factory::createNullBoxProc(b, loc, symTy)};
607-
b.create<fir::HasValueOp>(loc, box);
608-
});
600+
createGlobalInitialization(builder, global, [&](fir::FirOpBuilder &b) {
601+
auto box{fir::factory::createNullBoxProc(b, loc, symTy)};
602+
b.create<fir::HasValueOp>(loc, box);
603+
});
609604
}
610605
} else {
611606
// No initialization.
612-
Fortran::lower::createGlobalInitialization(
613-
builder, global, [&](fir::FirOpBuilder &b) {
614-
auto box{fir::factory::createNullBoxProc(b, loc, symTy)};
615-
b.create<fir::HasValueOp>(loc, box);
616-
});
607+
createGlobalInitialization(builder, global, [&](fir::FirOpBuilder &b) {
608+
auto box{fir::factory::createNullBoxProc(b, loc, symTy)};
609+
b.create<fir::HasValueOp>(loc, box);
610+
});
617611
}
618612
} else if (sym.has<Fortran::semantics::CommonBlockDetails>()) {
619613
mlir::emitError(loc, "COMMON symbol processed elsewhere");
@@ -634,7 +628,7 @@ static fir::GlobalOp defineGlobal(Fortran::lower::AbstractConverter &converter,
634628
// file.
635629
if (sym.attrs().test(Fortran::semantics::Attr::BIND_C))
636630
global.setLinkName(builder.createCommonLinkage());
637-
Fortran::lower::createGlobalInitialization(
631+
createGlobalInitialization(
638632
builder, global, [&](fir::FirOpBuilder &builder) {
639633
mlir::Value initValue;
640634
if (converter.getLoweringOptions().getInitGlobalZero())
@@ -826,7 +820,7 @@ void Fortran::lower::defaultInitializeAtRuntime(
826820
/*isConst=*/true,
827821
/*isTarget=*/false,
828822
/*dataAttr=*/{});
829-
Fortran::lower::createGlobalInitialization(
823+
createGlobalInitialization(
830824
builder, global, [&](fir::FirOpBuilder &builder) {
831825
Fortran::lower::StatementContext stmtCtx(
832826
/*cleanupProhibited=*/true);
@@ -842,7 +836,7 @@ void Fortran::lower::defaultInitializeAtRuntime(
842836
/*isConst=*/true,
843837
/*isTarget=*/false,
844838
/*dataAttr=*/{});
845-
Fortran::lower::createGlobalInitialization(
839+
createGlobalInitialization(
846840
builder, global, [&](fir::FirOpBuilder &builder) {
847841
Fortran::lower::StatementContext stmtCtx(
848842
/*cleanupProhibited=*/true);
@@ -1207,7 +1201,7 @@ static fir::GlobalOp defineGlobalAggregateStore(
12071201
if (const auto *objectDetails =
12081202
initSym->detailsIf<Fortran::semantics::ObjectEntityDetails>())
12091203
if (objectDetails->init()) {
1210-
Fortran::lower::createGlobalInitialization(
1204+
createGlobalInitialization(
12111205
builder, global, [&](fir::FirOpBuilder &builder) {
12121206
Fortran::lower::StatementContext stmtCtx;
12131207
mlir::Value initVal = fir::getBase(genInitializerExprValue(
@@ -1219,12 +1213,11 @@ static fir::GlobalOp defineGlobalAggregateStore(
12191213
// Equivalence has no Fortran initial value. Create an undefined FIR initial
12201214
// value to ensure this is consider an object definition in the IR regardless
12211215
// of the linkage.
1222-
Fortran::lower::createGlobalInitialization(
1223-
builder, global, [&](fir::FirOpBuilder &builder) {
1224-
Fortran::lower::StatementContext stmtCtx;
1225-
mlir::Value initVal = builder.create<fir::ZeroOp>(loc, aggTy);
1226-
builder.create<fir::HasValueOp>(loc, initVal);
1227-
});
1216+
createGlobalInitialization(builder, global, [&](fir::FirOpBuilder &builder) {
1217+
Fortran::lower::StatementContext stmtCtx;
1218+
mlir::Value initVal = builder.create<fir::ZeroOp>(loc, aggTy);
1219+
builder.create<fir::HasValueOp>(loc, initVal);
1220+
});
12281221
return global;
12291222
}
12301223

@@ -1543,7 +1536,7 @@ static void finalizeCommonBlockDefinition(
15431536
LLVM_DEBUG(llvm::dbgs() << "}\n");
15441537
builder.create<fir::HasValueOp>(loc, cb);
15451538
};
1546-
Fortran::lower::createGlobalInitialization(builder, global, initFunc);
1539+
createGlobalInitialization(builder, global, initFunc);
15471540
}
15481541

15491542
void Fortran::lower::defineCommonBlocks(

‎flang/lib/Lower/OpenMP/OpenMP.cpp‎

Lines changed: 1 addition & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -662,32 +662,9 @@ static fir::GlobalOp globalInitialization(lower::AbstractConverter &converter,
662662
const semantics::Symbol &sym,
663663
const lower::pft::Variable &var,
664664
mlir::Location currentLocation) {
665-
mlir::Type ty = converter.genType(sym);
666665
std::string globalName = converter.mangleName(sym);
667666
mlir::StringAttr linkage = firOpBuilder.createInternalLinkage();
668-
fir::GlobalOp global =
669-
firOpBuilder.createGlobal(currentLocation, ty, globalName, linkage);
670-
671-
// Create default initialization for non-character scalar.
672-
if (semantics::IsAllocatableOrObjectPointer(&sym)) {
673-
mlir::Type baseAddrType = mlir::dyn_cast<fir::BoxType>(ty).getEleTy();
674-
lower::createGlobalInitialization(
675-
firOpBuilder, global, [&](fir::FirOpBuilder &b) {
676-
mlir::Value nullAddr =
677-
b.createNullConstant(currentLocation, baseAddrType);
678-
mlir::Value box =
679-
b.create<fir::EmboxOp>(currentLocation, ty, nullAddr);
680-
b.create<fir::HasValueOp>(currentLocation, box);
681-
});
682-
} else {
683-
lower::createGlobalInitialization(
684-
firOpBuilder, global, [&](fir::FirOpBuilder &b) {
685-
mlir::Value undef = b.create<fir::UndefOp>(currentLocation, ty);
686-
b.create<fir::HasValueOp>(currentLocation, undef);
687-
});
688-
}
689-
690-
return global;
667+
return Fortran::lower::defineGlobal(converter, var, globalName, linkage);
691668
}
692669

693670
// Get the extended value for \p val by extracting additional variable

‎flang/test/Lower/OpenMP/omp-declare-target-program-var.f90‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ PROGRAM main
66
! HOST-DAG: %[[I_DECL:.*]]:2 = hlfir.declare %[[I_REF]] {uniq_name = "_QFEi"} : (!fir.ref<f32>) -> (!fir.ref<f32>, !fir.ref<f32>)
77
REAL :: I
88
! ALL-DAG: fir.global internal @_QFEi {omp.declare_target = #omp.declaretarget<device_type = (any), capture_clause = (to)>} : f32 {
9-
! ALL-DAG: %[[UNDEF:.*]] = fir.undefined f32
9+
! ALL-DAG: %[[UNDEF:.*]] = fir.zero_bits f32
1010
! ALL-DAG: fir.has_value %[[UNDEF]] : f32
1111
! ALL-DAG: }
1212
!$omp declare target(I)

‎flang/test/Lower/OpenMP/threadprivate-host-association-2.f90‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
!CHECK: return
2828
!CHECK: }
2929
!CHECK: fir.global internal @_QFEa : i32 {
30-
!CHECK: %[[A:.*]] = fir.undefined i32
30+
!CHECK: %[[A:.*]] = fir.zero_bits i32
3131
!CHECK: fir.has_value %[[A]] : i32
3232
!CHECK: }
3333

‎flang/test/Lower/OpenMP/threadprivate-host-association-3.f90‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
!CHECK: return
2828
!CHECK: }
2929
!CHECK: fir.global internal @_QFEa : i32 {
30-
!CHECK: %[[A:.*]] = fir.undefined i32
30+
!CHECK: %[[A:.*]] = fir.zero_bits i32
3131
!CHECK: fir.has_value %[[A]] : i32
3232
!CHECK: }
3333

‎flang/test/Lower/OpenMP/threadprivate-lenparams.f90‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
! RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s
2+
3+
! Regression test for https://github.com/llvm/llvm-project/issues/108136
4+
5+
character(:), pointer :: c
6+
character(2), pointer :: c2
7+
!$omp threadprivate(c, c2)
8+
end
9+
10+
! CHECK-LABEL: fir.global internal @_QFEc : !fir.box<!fir.ptr<!fir.char<1,?>>> {
11+
! CHECK: %[[VAL_0:.*]] = fir.zero_bits !fir.ptr<!fir.char<1,?>>
12+
! CHECK: %[[VAL_1:.*]] = arith.constant 0 : index
13+
! CHECK: %[[VAL_2:.*]] = fir.embox %[[VAL_0]] typeparams %[[VAL_1]] : (!fir.ptr<!fir.char<1,?>>, index) -> !fir.box<!fir.ptr<!fir.char<1,?>>>
14+
! CHECK: fir.has_value %[[VAL_2]] : !fir.box<!fir.ptr<!fir.char<1,?>>>
15+
! CHECK: }
16+
17+
! CHECK-LABEL: fir.global internal @_QFEc2 : !fir.box<!fir.ptr<!fir.char<1,2>>> {
18+
! CHECK: %[[VAL_0:.*]] = fir.zero_bits !fir.ptr<!fir.char<1,2>>
19+
! CHECK: %[[VAL_1:.*]] = fir.embox %[[VAL_0]] : (!fir.ptr<!fir.char<1,2>>) -> !fir.box<!fir.ptr<!fir.char<1,2>>>
20+
! CHECK: fir.has_value %[[VAL_1]] : !fir.box<!fir.ptr<!fir.char<1,2>>>
21+
! CHECK: }
22+

‎flang/test/Lower/OpenMP/threadprivate-non-global.f90‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -85,19 +85,19 @@ program test
8585
!CHECK-DAG: fir.has_value [[E1]] : !fir.box<!fir.heap<f32>>
8686
!CHECK-DAG: }
8787
!CHECK-DAG: fir.global internal @_QFEw : complex<f32> {
88-
!CHECK-DAG: [[Z2:%.*]] = fir.undefined complex<f32>
88+
!CHECK-DAG: [[Z2:%.*]] = fir.zero_bits complex<f32>
8989
!CHECK-DAG: fir.has_value [[Z2]] : complex<f32>
9090
!CHECK-DAG: }
9191
!CHECK-DAG: fir.global internal @_QFEx : i32 {
92-
!CHECK-DAG: [[Z3:%.*]] = fir.undefined i32
92+
!CHECK-DAG: [[Z3:%.*]] = fir.zero_bits i32
9393
!CHECK-DAG: fir.has_value [[Z3]] : i32
9494
!CHECK-DAG: }
9595
!CHECK-DAG: fir.global internal @_QFEy : f32 {
96-
!CHECK-DAG: [[Z4:%.*]] = fir.undefined f32
96+
!CHECK-DAG: [[Z4:%.*]] = fir.zero_bits f32
9797
!CHECK-DAG: fir.has_value [[Z4]] : f32
9898
!CHECK-DAG: }
9999
!CHECK-DAG: fir.global internal @_QFEz : !fir.logical<4> {
100-
!CHECK-DAG: [[Z5:%.*]] = fir.undefined !fir.logical<4>
100+
!CHECK-DAG: [[Z5:%.*]] = fir.zero_bits !fir.logical<4>
101101
!CHECK-DAG: fir.has_value [[Z5]] : !fir.logical<4>
102102
!CHECK-DAG: }
103103
end

0 commit comments

Comments
 (0)