[mlir][llvm] Modernize the import of LLVM IR globals.

[gysit]

Summary

Return failure if the import of a global variable fails and add a
test case to check the emitted error message. Additionally, convert
the globals in iteration order and do not process them recursively
when translating a constant expression referencing it. Additionally,
use the module location rather unknown location.

[Dinistro]

LGTM module the unnecessary global mapping.

mlir/include/mlir/Target/LLVMIR/ModuleImport.h

116–128

I think we do not need this mapping at this point. Other globals are reference with the symbol name.

249–250

As above

[gysit]

Address review comments.

mlir/include/mlir/Target/LLVMIR/ModuleImport.h

public:
   }
 
   /// Returns the MLIR value mapped to the given LLVM value.
   Value lookupValue(llvm::Value *value) { return valueMapping.lookup(value); }
 
   /// Stores a mapping between an LLVM instruction and the imported MLIR
   /// operation if the operation returns no result. Asserts if the operation
   /// returns a result and should be added to valueMapping instead.
-  void mapNoResultOp(llvm::Instruction *inst, Operation *mlir) {
-    mapNoResultOp(inst) = mlir;
+  void mapNoResultOp(llvm::Instruction *llvm, Operation *mlir) {
+    mapNoResultOp(llvm) = mlir;
   }
 
   /// Provides write-once access to store the MLIR operation corresponding to
   /// the given LLVM instruction if the operation returns no result. Asserts if
   /// the operation returns a result and should be added to valueMapping
   /// instead.
   Operation *&mapNoResultOp(llvm::Instruction *inst) {
     Operation *&mlir = noResultOpMapping[inst];
void mapBlock(llvm::BasicBlock *llvm, Block *mlir) {
     assert(result.second && "attempting to map a block that is already mapped");
   }
 
   /// Returns the MLIR block mapped to the given LLVM block.
   Block *lookupBlock(llvm::BasicBlock *block) const {
     return blockMapping.lookup(block);
   }
 
   /// Converts an LLVM value to an MLIR value, or returns failure if the
   /// conversion fails. Uses the `convertConstant` method to translate constant
   /// LLVM values.
   FailureOr<Value> convertValue(llvm::Value *value);
 
   /// Converts a range of LLVM values to a range of MLIR values using the
   /// `convertValue` method, or returns failure if the conversion fails.
   FailureOr<SmallVector<Value>> convertValues(ArrayRef<llvm::Value *> values);
 
   /// Converts `value` to an integer attribute. Asserts if the matching fails.
   IntegerAttr matchIntegerAttr(llvm::Value *value);
 
   /// Converts `value` to a local variable attribute. Asserts if the matching
   /// fails.
   DILocalVariableAttr matchLocalVariableAttr(llvm::Value *value);
 
   /// Translates the debug location.
   Location translateLoc(llvm::DILocation *loc);
 
   /// Converts the type from LLVM to MLIR LLVM dialect.
   Type convertType(llvm::Type *type) {
     return typeTranslator.translateType(type);
   }
 
   /// Imports `func` into the current module.
   LogicalResult processFunction(llvm::Function *func);
 
-  /// Converts function attributes of LLVM Function \p func
-  /// into LLVM dialect attributes of LLVMFuncOp \p funcOp.
+  /// Converts function attributes of LLVM Function `func` into LLVM dialect
+  /// attributes of LLVMFuncOp `funcOp`.
   void processFunctionAttributes(llvm::Function *func, LLVMFuncOp funcOp);
 
-  /// Imports `globalVar` as a GlobalOp, creating it if it doesn't exist.
-  GlobalOp processGlobal(llvm::GlobalVariable *globalVar);
-
   /// Sets the fastmath flags attribute for the imported operation `op` given
   /// the original instruction `inst`. Asserts if the operation does not
   /// implement the fastmath interface.
   void setFastmathFlagsAttr(llvm::Instruction *inst, Operation *op) const;
 
 private:
   /// Clears the block and value mapping before processing a new region.
   void clearBlockAndValueMapping() {
     valueMapping.clear();
     noResultOpMapping.clear();
     blockMapping.clear();
   }
   /// Sets the constant insertion point to the start of the given block.
   void setConstantInsertionPointToStart(Block *block) {
     constantInsertionBlock = block;
     constantInsertionOp = nullptr;
   }
 
+  /// Converts an LLVM global variable into an MLIR LLVM dialect global
+  /// operation if a conversion exists. Otherwise, returns failure.
+  LogicalResult convertGlobal(llvm::GlobalVariable *globalVar);
+  /// Imports the magic globals "global_ctors" and "global_dtors".
+  LogicalResult convertGlobalCtorsAndDtors(llvm::GlobalVariable *globalVar);
   /// Returns personality of `func` as a FlatSymbolRefAttr.
   FlatSymbolRefAttr getPersonalityAsAttr(llvm::Function *func);
   /// Imports `bb` into `block`, which must be initially empty.
   LogicalResult processBasicBlock(llvm::BasicBlock *bb, Block *block);
   /// Converts an LLVM intrinsic to an MLIR LLVM dialect operation if an MLIR
   /// counterpart exists. Otherwise, returns failure.
   LogicalResult convertIntrinsic(OpBuilder &odsBuilder, llvm::CallInst *inst);
   /// Converts an LLVM instruction to an MLIR LLVM dialect operation if an MLIR
private:
   FailureOr<Value> convertConstant(llvm::Constant *constant);
   /// Converts an LLVM constant and its transitive constant dependencies to MLIR
   /// operations by converting them in topological order using the
   /// `convertConstant` method, or returns failure if the conversion of any of
   /// them fails. All operations are inserted at the start of the current
   /// function entry block.
   FailureOr<Value> convertConstantExpr(llvm::Constant *constant);
 
-  /// Imports the magic globals "global_ctors" and "global_dtors".
-  LogicalResult convertGlobalCtorsAndDtors(llvm::GlobalVariable *globalVar);
-
   /// Builder pointing at where the next instruction should be generated.
   OpBuilder builder;
   /// Block to insert the next constant into.
   Block *constantInsertionBlock = nullptr;
   /// Operation to insert the next constant after.
   Operation *constantInsertionOp = nullptr;
   /// Operation to insert the next global after.
   Operation *globalInsertionOp = nullptr;
private:
 
   /// Function-local mapping between original and imported block.
   DenseMap<llvm::BasicBlock *, Block *> blockMapping;
   /// Function-local mapping between original and imported values.
   DenseMap<llvm::Value *, Value> valueMapping;
   /// Function-local mapping between original instructions and imported
   /// operations for all operations that return no result. All operations that
   /// return a result have a valueMapping entry instead.
   DenseMap<llvm::Instruction *, Operation *> noResultOpMapping;
-  /// Uniquing map of GlobalVariables.
-  DenseMap<llvm::GlobalVariable *, GlobalOp> globals;
   /// The stateful type translator (contains named structs).
   LLVM::TypeFromLLVMIRTranslator typeTranslator;
   /// Stateful debug information importer.
   std::unique_ptr<detail::DebugImporter> debugImporter;
 };
 
 } // namespace LLVM
 } // namespace mlir
 
 #endif // MLIR_TARGET_LLVMIR_MODULEIMPORT_H

mlir/lib/Target/LLVMIR/ModuleImport.cpp

for (llvm::GlobalVariable &globalVar : llvmModule->globals()) {
     if (globalVar.getName() == getGlobalCtorsVarName() ||
         globalVar.getName() == getGlobalDtorsVarName()) {
       if (failed(convertGlobalCtorsAndDtors(&globalVar))) {
         return emitError(mlirModule.getLoc())
                << "unhandled global variable " << diag(globalVar);
       }
       continue;
     }
-
-    if (!processGlobal(&globalVar))
-      return failure();
+    if (failed(convertGlobal(&globalVar))) {
+      return emitError(mlirModule.getLoc())
+             << "unhandled global variable " << diag(globalVar);
+    }
   }
   return success();
 }
 
 LogicalResult ModuleImport::convertFunctions() {
   for (llvm::Function &func : llvmModule->functions())
     if (failed(processFunction(&func)))
       return failure();
if (isa<llvm::ConstantAggregate>(value)) {
     }
 
     return DenseElementsAttr::get(outerType, values);
   }
 
   return nullptr;
 }
 
-GlobalOp ModuleImport::processGlobal(llvm::GlobalVariable *globalVar) {
-  if (globals.count(globalVar))
-    return globals[globalVar];
-
+LogicalResult ModuleImport::convertGlobal(llvm::GlobalVariable *globalVar) {
   // Insert the global after the last one or at the start of the module.
   OpBuilder::InsertionGuard guard(builder);
-  if (!globalInsertionOp) {
+  if (!globalInsertionOp)
     builder.setInsertionPointToStart(mlirModule.getBody());
-  } else {
+  else
     builder.setInsertionPointAfter(globalInsertionOp);
-  }
 
   Attribute valueAttr;
   if (globalVar->hasInitializer())
     valueAttr = getConstantAsAttr(globalVar->getInitializer());
   Type type = convertType(globalVar->getValueType());
 
   uint64_t alignment = 0;
   llvm::MaybeAlign maybeAlign = globalVar->getAlign();
   if (maybeAlign.has_value()) {
     llvm::Align align = *maybeAlign;
     alignment = align.value();
   }
 
   GlobalOp globalOp = builder.create<GlobalOp>(
-      UnknownLoc::get(context), type, globalVar->isConstant(),
+      mlirModule.getLoc(), type, globalVar->isConstant(),
       convertLinkageFromLLVM(globalVar->getLinkage()), globalVar->getName(),
       valueAttr, alignment, /*addr_space=*/globalVar->getAddressSpace(),
       /*dso_local=*/globalVar->isDSOLocal(),
       /*thread_local=*/globalVar->isThreadLocal());
   globalInsertionOp = globalOp;
 
   if (globalVar->hasInitializer() && !valueAttr) {
     clearBlockAndValueMapping();
     Block *block = builder.createBlock(&globalOp.getInitializerRegion());
     setConstantInsertionPointToStart(block);
     FailureOr<Value> initializer =
         convertConstantExpr(globalVar->getInitializer());
     if (failed(initializer))
-      return {};
+      return failure();
     builder.create<ReturnOp>(globalOp.getLoc(), *initializer);
   }
   if (globalVar->hasAtLeastLocalUnnamedAddr()) {
     globalOp.setUnnamedAddr(
         convertUnnamedAddrFromLLVM(globalVar->getUnnamedAddr()));
   }
   if (globalVar->hasSection())
     globalOp.setSection(globalVar->getSection());
 
-  return globals[globalVar] = globalOp;
+  return success();
 }
 
 LogicalResult
 ModuleImport::convertGlobalCtorsAndDtors(llvm::GlobalVariable *globalVar) {
   if (!globalVar->hasInitializer() || !globalVar->hasAppendingLinkage())
     return failure();
   auto *initializer =
       dyn_cast<llvm::ConstantArray>(globalVar->getInitializer());
FailureOr<Value> ModuleImport::convertConstant(llvm::Constant *constant) {
   // Convert undef.
   if (auto *undefVal = dyn_cast<llvm::UndefValue>(constant)) {
     Type type = convertType(undefVal->getType());
     return builder.create<UndefOp>(loc, type).getResult();
   }
 
   // Convert global variable accesses.
   if (auto *globalVar = dyn_cast<llvm::GlobalVariable>(constant)) {
-    return builder.create<AddressOfOp>(loc, processGlobal(globalVar))
-        .getResult();
+    Type type = convertType(globalVar->getType());
+    auto symbolRef = FlatSymbolRefAttr::get(context, globalVar->getName());
+    return builder.create<AddressOfOp>(loc, type, symbolRef).getResult();
   }
 
   // Convert constant expressions.
   if (auto *constExpr = dyn_cast<llvm::ConstantExpr>(constant)) {
     // Convert the constant expression to a temporary LLVM instruction and
     // translate it using the `processInstruction` method. Delete the
     // instruction after the translation and remove it from `valueMapping`,
     // since later calls to `getAsInstruction` may return the same address

 }
 
 FailureOr<Value> ModuleImport::convertConstantExpr(llvm::Constant *constant) {
   assert(constantInsertionBlock &&
          "expected the constant insertion block to be non-null");
 
   // Insert the constant after the last one or at the start or the entry block.
   OpBuilder::InsertionGuard guard(builder);
-  if (!constantInsertionOp) {
+  if (!constantInsertionOp)
     builder.setInsertionPointToStart(constantInsertionBlock);
-  } else {
+  else
     builder.setInsertionPointAfter(constantInsertionOp);
-  }
 
   // Convert all constants of the expression and add them to `valueMapping`.
   SetVector<llvm::Constant *> constantsToConvert =
       getConstantsToConvert(constant);
   for (llvm::Constant *constantToConvert : constantsToConvert) {
     FailureOr<Value> converted = convertConstant(constantToConvert);
     if (failed(converted))
       return failure();

mlir/test/Target/LLVMIR/Import/global-variables.ll

 ; RUN: mlir-translate -import-llvm -split-input-file %s | FileCheck %s
 
 %sub_struct = type {}
 %my_struct = type { %sub_struct, i64 }
 
 ; CHECK:  llvm.mlir.global external @global_struct
 ; CHECK-SAME:  {addr_space = 0 : i32, alignment = 8 : i64}
 ; CHECK-SAME:  !llvm.struct<"my_struct", (struct<"sub_struct", ()>, i64)>
 @global_struct = external global %my_struct, align 8
 
 ; CHECK:  llvm.mlir.global external @global_float
 ; CHECK-SAME:  {addr_space = 0 : i32, alignment = 8 : i64} : f64
 @global_float = external global double, align 8
 
+; CHECK:  llvm.mlir.global internal constant @address_before
+; CHECK:  = llvm.mlir.addressof @global_int : !llvm.ptr<i32>
+@address_before = internal constant i32* @global_int
+
 ; CHECK:  llvm.mlir.global external @global_int
 ; CHECK-SAME:  {addr_space = 0 : i32, alignment = 8 : i64} : i32
 @global_int = external global i32, align 8
 
+; CHECK:  llvm.mlir.global internal constant @address_after
+; CHECK:  = llvm.mlir.addressof @global_int : !llvm.ptr<i32>
+@address_after = internal constant i32* @global_int
+
 ; CHECK:  llvm.mlir.global internal @global_string("hello world")
 @global_string = internal global [11 x i8] c"hello world"
 
 ; CHECK:  llvm.mlir.global external @global_vector
 ; CHECK-SAME:  {addr_space = 0 : i32} : vector<8xi32>
 @global_vector = external global <8 x i32>
 
 ; CHECK: llvm.mlir.global internal constant @global_gep_const_expr

mlir/test/Target/LLVMIR/Import/import-failure.ll

 ; CHECK: error: unhandled constant i8* blockaddress(@unhandled_constant, %bb1)
 define i8* @unhandled_constant() {
 bb1:
   ret i8* blockaddress(@unhandled_constant, %bb1)
 }
 
 ; // -----
 
+; CHECK: error: unhandled constant i8* blockaddress(@unhandled_global, %bb1)
+; CHECK: error: unhandled global variable @private = private global i8* blockaddress(@unhandled_global, %bb1)
+@private = private global i8* blockaddress(@unhandled_global, %bb1)
+
+define void @unhandled_global() {
+bb1:
+  ret void
+}
+
+; // -----
+
 declare void @llvm.gcroot(ptr %arg0, ptr %arg1)
 
 ; CHECK: error: unhandled intrinsic call void @llvm.gcroot(ptr %arg0, ptr %arg1)
 define void @unhandled_intrinsic(ptr %arg0, ptr %arg1) {
   call void @llvm.gcroot(ptr %arg0, ptr %arg1)
   ret void
 }
 
define void @dropped_instruction(i64 %arg1) {
   call void @llvm.dbg.value(metadata i64 %arg1, metadata !3, metadata !DIExpression(DW_OP_plus_uconst, 42, DW_OP_stack_value)), !dbg !5
   ret void
 }
 
 !llvm.dbg.cu = !{!1}
 !llvm.module.flags = !{!0}
 !0 = !{i32 2, !"Debug Info Version", i32 3}
 !1 = distinct !DICompileUnit(language: DW_LANG_C, file: !2)
-!2 = !DIFile(filename: "debug-info.ll", directory: "/")
+!2 = !DIFile(filename: "import-failure.ll", directory: "/")
 !3 = !DILocalVariable(scope: !4, name: "arg", file: !2, line: 1, arg: 1, align: 32);
 !4 = distinct !DISubprogram(name: "intrinsic", scope: !2, file: !2, spFlags: DISPFlagDefinition, unit: !1)
 !5 = !DILocation(line: 1, column: 2, scope: !4)
 
 ; // -----
 
 ; global_ctors requires the appending linkage type.
 ; CHECK: error: unhandled global variable @llvm.global_ctors