[mlir] don't drop undef initializers in translation to LLVM IR

[ftynse]

Summary

LLVM IR allows globals with external linkage to have initializers, including
undef. The translation was incorrectly using undef as a indicator that the
initializer should be ignored in translation, leading to the impossibility to
create an external global with an explicit undef initializer. Fix this and use
nullptr as a marker instead.

[wsmoses]

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

458

This comment isn't 100% correct. My understanding of the LLVM semantics is that for an external linkage variable if you have an initializer the symbol is externally visible and defined within the module, if you do not have an initializer the symbol is defined in another translation unit and available here.

As such I don't think we should be assigning the initializer by default (since the explicit inclusion or omission of the initializer is what defines the semantics).

mlir/test/Target/LLVMIR/llvmir.mlir

38–44

Per earlier comment I would also add a test which explicitly doesn't have an initializer in mlir and ensure it does not in llvm either.

[wsmoses]

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

458

Ignore my previous comment about not assigning undef by default, rereading this it should only occur for non-external variables. The comment here is fine, but I may recommend adding to the comment for shouldDropGlobalInitializer with the previous explanation.

mlir/test/Target/LLVMIR/llvmir.mlir

38–44

Ignore this comment, I'm seeing this behavior checked on line 78.

[ftynse]

Added more comments before landing.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

 
 /// A helper method to get the single Block in an operation honoring LLVM's
 /// module requirements.
 static Block &getModuleBody(Operation *module) {
   return module->getRegion(0).front();
 }
 
 /// 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;
 }
 
 /// Sets the runtime preemption specifier of `gv` to dso_local if
 /// `dsoLocalRequested` is true, otherwise it is left unchanged.
 static void addRuntimePreemptionSpecifier(bool dsoLocalRequested,
                                           llvm::GlobalValue *gv) {
   if (dsoLocalRequested)
     gv->setDSOLocal(true);
 }
 
 /// Create named global variables that correspond to llvm.mlir.global
 /// definitions.
 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.
       if (auto strAttr = op.getValueOrNull().dyn_cast_or_null<StringAttr>()) {
         cst = llvm::ConstantDataArray::getString(
             llvmModule->getContext(), strAttr.getValue(), /*AddNull=*/false);
         type = cst->getType();
       } else if (!(cst = getLLVMConstant(type, op.getValueOrNull(), op.getLoc(),
                                          *this))) {
         return failure();
       }
     }
 
     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())
       var->setUnnamedAddr(convertUnnamedAddrToLLVM(*op.unnamed_addr()));
 
     if (op.section().hasValue())
       var->setSection(*op.section());
 

mlir/test/Target/LLVMIR/llvmir.mlir

 // CHECK: @float_global_array = internal global [1 x float] [float -5.000000e+00]
 llvm.mlir.global internal @float_global_array(dense<[-5.0]> : vector<1xf32>) : !llvm.array<1 x f32>
 
 // CHECK: @string_const = internal constant [6 x i8] c"foobar"
 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>
   %_c0 = llvm.mlir.constant(2: i32) :i32
   %gepinit = llvm.getelementptr %addr[%_c0] : (!llvm.ptr<i32>, i32) -> !llvm.ptr<i32>
   llvm.return %gepinit : !llvm.ptr<i32>
 }