Merge pull request #1102 from Leporacanthicus/fix-opt-globals

schweitzpgi · web-flow · commit 06af2071dbc9 · 2021-09-30T10:27:57.000-07:00
[flang]Fix linkage of globals
diff --git a/flang/lib/Lower/ConvertVariable.cpp b/flang/lib/Lower/ConvertVariable.cpp
@@ -1650,14 +1650,9 @@ void Fortran::lower::mapSymbolAttributes(
 
 void Fortran::lower::defineModuleVariable(
     AbstractConverter &converter, const Fortran::lower::pft::Variable &var) {
-  // linkOnce linkage allows the definition to be kept by LLVM
-  // even if it is unused and there is not init (other than undef).
-  // Emitting llvm :'@glob = global type undef' would also work, but mlir
-  // always insert "external" and removes the undef init when lowering a
-  // global without explicit linkage. This ends-up in llvm removing the
-  // symbol if unsued potentially creating linking issues. Hence the use
-  // of linkOnce.
-  auto linkOnce = converter.getFirOpBuilder().createLinkOnceLinkage();
+  // Use empty linkage for module variables, which makes them available
+  // for use in another unit.
+  mlir::StringAttr externalLinkage;
   // Only define variable owned by this module
   if (var.isDeclaration())
     return;
@@ -1668,7 +1663,7 @@ void Fortran::lower::defineModuleVariable(
   if (var.isAggregateStore()) {
     auto &aggregate = var.getAggregateStore();
     auto aggName = mangleGlobalAggregateStore(aggregate);
-    defineGlobalAggregateStore(converter, aggregate, aggName, linkOnce);
+    defineGlobalAggregateStore(converter, aggregate, aggName, externalLinkage);
     return;
   }
   const auto &sym = var.getSymbol();
@@ -1680,7 +1675,7 @@ void Fortran::lower::defineModuleVariable(
     // Do nothing. Mapping will be done on user side.
   } else {
     auto globalName = Fortran::lower::mangle::mangleName(sym);
-    defineGlobal(converter, var, globalName, linkOnce);
+    defineGlobal(converter, var, globalName, externalLinkage);
   }
 }
 
diff --git a/flang/test/Lower/allocatable-globals.f90 b/flang/test/Lower/allocatable-globals.f90
@@ -3,7 +3,7 @@
 
 ! Test global allocatable definition lowering
 
-! CHECK-LABEL: fir.global linkonce @_QMmod_allocatablesEc : !fir.box<!fir.heap<!fir.array<?x!fir.char<1,10>>>> {
+! CHECK-LABEL: fir.global @_QMmod_allocatablesEc : !fir.box<!fir.heap<!fir.array<?x!fir.char<1,10>>>> {
   ! CHECK-DAG: %[[modcNullAddr:.*]] = fir.zero_bits !fir.heap<!fir.array<?x!fir.char<1,10>>>
   ! CHECK-DAG: %[[modcShape:.*]] = fir.shape %c0{{.*}} : (index) -> !fir.shape<1>
   ! CHECK: %[[modcInitBox:.*]] = fir.embox %[[modcNullAddr]](%[[modcShape]]) : (!fir.heap<!fir.array<?x!fir.char<1,10>>>, !fir.shape<1>) -> !fir.box<!fir.heap<!fir.array<?x!fir.char<1,10>>>>
diff --git a/flang/test/Lower/default-initialization-globals.f90 b/flang/test/Lower/default-initialization-globals.f90
@@ -57,15 +57,15 @@ module tinit
 
   ! Test scalar with default init
   type(t0) :: at0
-! CHECK-LABEL: fir.global linkonce @_QMtinitEat0 : !fir.type<_QMtinitTt0{k:i32}> {
+! CHECK-LABEL: fir.global @_QMtinitEat0 : !fir.type<_QMtinitTt0{k:i32}> {
   ! CHECK: %[[VAL_0:.*]] = constant 66 : i32
   ! CHECK: %[[VAL_1:.*]] = fir.undefined !fir.type<_QMtinitTt0{k:i32}>
   ! CHECK: %[[VAL_2:.*]] = fir.insert_value %[[VAL_1]], %[[VAL_0]], ["k", !fir.type<_QMtinitTt0{k:i32}>] : (!fir.type<_QMtinitTt0{k:i32}>, i32) -> !fir.type<_QMtinitTt0{k:i32}>
   ! CHECK: fir.has_value %[[VAL_2]] : !fir.type<_QMtinitTt0{k:i32}>
 
   ! Test array with default init
   type(t0) :: bt0(100)
-! CHECK-LABEL: linkonce @_QMtinitEbt0 : !fir.array<100x!fir.type<_QMtinitTt0{k:i32}>> {
+! CHECK-LABEL: @_QMtinitEbt0 : !fir.array<100x!fir.type<_QMtinitTt0{k:i32}>> {
   ! CHECK: %[[VAL_3:.*]] = constant 66 : i32
   ! CHECK: %[[VAL_4:.*]] = fir.undefined !fir.type<_QMtinitTt0{k:i32}>
   ! CHECK: %[[VAL_5:.*]] = fir.insert_value %[[VAL_4]], %[[VAL_3]], ["k", !fir.type<_QMtinitTt0{k:i32}>] : (!fir.type<_QMtinitTt0{k:i32}>, i32) -> !fir.type<_QMtinitTt0{k:i32}>
@@ -75,15 +75,15 @@ module tinit
 
   ! Test default init overridden by explicit init
   type(t0) :: ct0 = t0(42)
-! CHECK-LABEL: fir.global linkonce @_QMtinitEct0 : !fir.type<_QMtinitTt0{k:i32}> {
+! CHECK-LABEL: fir.global @_QMtinitEct0 : !fir.type<_QMtinitTt0{k:i32}> {
   ! CHECK: %[[VAL_8:.*]] = constant 42 : i32
   ! CHECK: %[[VAL_9:.*]] = fir.undefined !fir.type<_QMtinitTt0{k:i32}>
   ! CHECK: %[[VAL_10:.*]] = fir.insert_value %[[VAL_9]], %[[VAL_8]], ["k", !fir.type<_QMtinitTt0{k:i32}>] : (!fir.type<_QMtinitTt0{k:i32}>, i32) -> !fir.type<_QMtinitTt0{k:i32}>
   ! CHECK: fir.has_value %[[VAL_10]] : !fir.type<_QMtinitTt0{k:i32}>
 
   ! Test a non trivial derived type mixing all sorts of default initialization
   type(t1) :: dt1
-! CHECK-LABEL: linkonce @_QMtinitEdt1 : !fir.type<_QMtinitTt1{{.*}}> {
+! CHECK-LABEL: @_QMtinitEdt1 : !fir.type<_QMtinitTt1{{.*}}> {
   ! CHECK-DAG: %[[VAL_11:.*]] = constant 42 : i32
   ! CHECK-DAG: %[[VAL_12:.*]] = constant 100 : index
   ! CHECK-DAG: %[[VAL_13:.*]] = constant 0 : index
@@ -118,7 +118,7 @@ module tinit
 
   ! Test a type extending other type with a default init
   type(textendst0) :: etextendst0
-! CHECK-LABEL: linkonce @_QMtinitEetextendst0 : !fir.type<_QMtinitTtextendst0{k:i32,l:i32}> {
+! CHECK-LABEL: @_QMtinitEetextendst0 : !fir.type<_QMtinitTtextendst0{k:i32,l:i32}> {
   ! CHECK: %[[VAL_42:.*]] = constant 66 : i32
   ! CHECK: %[[VAL_43:.*]] = fir.undefined !fir.type<_QMtinitTtextendst0{k:i32,l:i32}>
   ! CHECK: %[[VAL_44:.*]] = fir.insert_value %[[VAL_43]], %[[VAL_42]], ["k", !fir.type<_QMtinitTtextendst0{k:i32,l:i32}>] : (!fir.type<_QMtinitTtextendst0{k:i32,l:i32}>, i32) -> !fir.type<_QMtinitTtextendst0{k:i32,l:i32}>
diff --git a/flang/test/Lower/derived-types.f90 b/flang/test/Lower/derived-types.f90
@@ -190,7 +190,7 @@ module data_mod
 
 ! Test globals
 
-! CHECK-DAG: fir.global linkonce @_QMdata_modEsome_c2 : !fir.type<_QMdTc2{ch_array:!fir.array<20x30x!fir.char<1,10>>}>
-! CHECK-DAG: fir.global linkonce @_QMdata_modEsome_r : !fir.type<_QMdTr{x:f32}>
+! CHECK-DAG: fir.global @_QMdata_modEsome_c2 : !fir.type<_QMdTc2{ch_array:!fir.array<20x30x!fir.char<1,10>>}>
+! CHECK-DAG: fir.global @_QMdata_modEsome_r : !fir.type<_QMdTr{x:f32}>
 ! CHECK-DAG: fir.global internal @_QMdFsaved_derivedEsome_c2 : !fir.type<_QMdTc2{ch_array:!fir.array<20x30x!fir.char<1,10>>}>
 ! CHECK-DAG: fir.global internal @_QMdFsaved_derivedEsome_r : !fir.type<_QMdTr{x:f32}>
diff --git a/flang/test/Lower/module_definition.f90 b/flang/test/Lower/module_definition.f90
@@ -8,8 +8,8 @@ module m1
   real :: x
   integer :: y(100)
 end module
-! CHECK: fir.global linkonce @_QMm1Ex : f32
-! CHECK: fir.global linkonce @_QMm1Ey : !fir.array<100xi32>
+! CHECK: fir.global @_QMm1Ex : f32
+! CHECK: fir.global @_QMm1Ey : !fir.array<100xi32>
 
 ! Module modEq1 defines data that is equivalenced and not used in this
 ! file.
@@ -21,8 +21,8 @@ module modEq1
   real :: y2(10)
   equivalence (x1(1), x2(5), x3(10)), (y1, y2(5))
 end module
-! CHECK-LABEL: fir.global linkonce @_QMmodeq1Ex1 : !fir.array<76xi8>
-! CHECK-LABEL: fir.global linkonce @_QMmodeq1Ey1 : !fir.array<10xi32> {
+! CHECK-LABEL: fir.global @_QMmodeq1Ex1 : !fir.array<76xi8>
+! CHECK-LABEL: fir.global @_QMmodeq1Ey1 : !fir.array<10xi32> {
   ! CHECK: %[[undef:.*]] = fir.undefined !fir.array<10xi32>
   ! CHECK: %[[v1:.*]] = fir.insert_on_range %0, %c0{{.*}}, [0 : index, 3 : index] : (!fir.array<10xi32>, i32) -> !fir.array<10xi32>
   ! CHECK: %[[v2:.*]] = fir.insert_value %1, %c1109917696{{.*}}, [4 : index] : (!fir.array<10xi32>, i32) -> !fir.array<10xi32>
diff --git a/flang/test/Lower/pointer-initial-target-2.f90 b/flang/test/Lower/pointer-initial-target-2.f90
@@ -9,7 +9,7 @@
 module some_mod
   real, target :: x(100)
   real, pointer :: p(:) => x
-! CHECK-LABEL: fir.global linkonce @_QMsome_modEp : !fir.box<!fir.ptr<!fir.array<?xf32>>> {
+! CHECK-LABEL: fir.global @_QMsome_modEp : !fir.box<!fir.ptr<!fir.array<?xf32>>> {
   ! CHECK: %[[x:.*]] = fir.address_of(@_QMsome_modEx) : !fir.ref<!fir.array<100xf32>>
   ! CHECK: %[[shape:.*]] = fir.shape %c100{{.*}} : (index) -> !fir.shape<1>
   ! CHECK: %[[box:.*]] = fir.embox %[[x]](%[[shape]]) : (!fir.ref<!fir.array<100xf32>>, !fir.shape<1>) -> !fir.box<!fir.ptr<!fir.array<?xf32>>>
@@ -22,7 +22,7 @@ module some_mod_2
   common /com/ x, y
   save :: /com/
   real, pointer :: p(:) => y
-! CHECK-LABEL: fir.global linkonce @_QMsome_mod_2Ep : !fir.box<!fir.ptr<!fir.array<?xf32>>> {
+! CHECK-LABEL: fir.global @_QMsome_mod_2Ep : !fir.box<!fir.ptr<!fir.array<?xf32>>> {
   ! CHECK: %[[c:.*]] = fir.address_of(@_QBcom) : !fir.ref<!fir.array<1200xi8>>
   ! CHECK: %[[com:.*]] = fir.convert %[[c]] : (!fir.ref<!fir.array<1200xi8>>) -> !fir.ref<!fir.array<?xi8>>
   ! CHECK: %[[yRaw:.*]] = fir.coordinate_of %[[com]], %c400{{.*}} : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
diff --git a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
@@ -442,11 +442,13 @@ static Block &getModuleBody(Operation *module) {
 }
 
 /// A helper method to decide if a constant must not be set as a global variable
-/// initializer.
+/// initializer. For an external linkage variable, the variable with an
+/// initializer is considered externally visible and defined in this module, the
+/// variable without an initializer is externally available and is defined
+/// elsewhere.
 static bool shouldDropGlobalInitializer(llvm::GlobalValue::LinkageTypes linkage,
                                         llvm::Constant *cst) {
-  return (linkage == llvm::GlobalVariable::ExternalLinkage &&
-          isa<llvm::UndefValue>(cst)) ||
+  return (linkage == llvm::GlobalVariable::ExternalLinkage && !cst) ||
          linkage == llvm::GlobalVariable::ExternalWeakLinkage;
 }
 
@@ -455,7 +457,7 @@ static bool shouldDropGlobalInitializer(llvm::GlobalValue::LinkageTypes linkage,
 LogicalResult ModuleTranslation::convertGlobals() {
   for (auto op : getModuleBody(mlirModule).getOps<LLVM::GlobalOp>()) {
     llvm::Type *type = convertType(op.getType());
-    llvm::Constant *cst = llvm::UndefValue::get(type);
+    llvm::Constant *cst = nullptr;
     if (op.getValueOrNull()) {
       // String attributes are treated separately because they cannot appear as
       // in-function constants and are thus not supported by getLLVMConstant.
@@ -471,10 +473,18 @@ LogicalResult ModuleTranslation::convertGlobals() {
 
     auto linkage = convertLinkageToLLVM(op.linkage());
     auto addrSpace = op.addr_space();
+
+    // LLVM IR requires constant with linkage other than external or weak
+    // external to have initializers. If MLIR does not provide an initializer,
+    // default to undef.
+    bool dropInitializer = shouldDropGlobalInitializer(linkage, cst);
+    if (!dropInitializer && !cst)
+      cst = llvm::UndefValue::get(type);
+    else if (dropInitializer && cst)
+      cst = nullptr;
+
     auto *var = new llvm::GlobalVariable(
-        *llvmModule, type, op.constant(), linkage,
-        shouldDropGlobalInitializer(linkage, cst) ? nullptr : cst,
-        op.sym_name(),
+        *llvmModule, type, op.constant(), linkage, cst, op.sym_name(),
         /*InsertBefore=*/nullptr, llvm::GlobalValue::NotThreadLocal, addrSpace);
 
     if (op.unnamed_addr().hasValue())
diff --git a/mlir/test/Target/LLVMIR/llvmir.mlir b/mlir/test/Target/LLVMIR/llvmir.mlir
@@ -33,6 +33,12 @@ llvm.mlir.global internal constant @string_const("foobar") : !llvm.array<6 x i8>
 // CHECK: @int_global_undef = internal global i64 undef
 llvm.mlir.global internal @int_global_undef() : i64
 
+// CHECK: @explicit_undef = global i32 undef
+llvm.mlir.global external @explicit_undef() : i32 {
+  %0 = llvm.mlir.undef : i32
+  llvm.return %0 : i32
+}
+
 // CHECK: @int_gep = internal constant i32* getelementptr (i32, i32* @i32_global, i32 2)
 llvm.mlir.global internal constant @int_gep() : !llvm.ptr<i32> {
   %addr = llvm.mlir.addressof @i32_global : !llvm.ptr<i32>
