[mlir] error out on unsupported attribute kinds in LLVM global translation

[ftynse]

Summary

Some attribute kinds are not supported as "value" attributes of
llvm.mlir.constant when translating to LLVM IR. We were correctly reporting
an error, but continuing the translation using an "undef" value instead,
leading to a surprising mix of error messages and output IR. Abort the
translation after the error is reported.

[rriddle]

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

435

nit: merge the if into the else

mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h

if (!satisfiesLLVMModule(m))
       return nullptr;
     if (failed(checkSupportedModuleOps(m)))
       return nullptr;
     auto llvmModule = prepareLLVMModule(m);
     if (!llvmModule)
       return nullptr;
 
     T translator(m, std::move(llvmModule));
-    translator.convertGlobals();
+    if (failed(translator.convertGlobals()))
+      return nullptr;
     if (failed(translator.convertFunctions()))
       return nullptr;
 
     return std::move(translator.llvmModule);
   }
 
   /// A helper method to get the single Block in an operation honoring LLVM's
   /// module requirements.
protected:
   /// A helper to look up remapped operands in the value remapping table.
   SmallVector<llvm::Value *, 8> lookupValues(ValueRange values);
 
 private:
   /// Check whether the module contains only supported ops directly in its body.
   static LogicalResult checkSupportedModuleOps(Operation *m);
 
   LogicalResult convertFunctions();
-  void convertGlobals();
+  LogicalResult convertGlobals();
   LogicalResult convertOneFunction(LLVMFuncOp func);
   void connectPHINodes(LLVMFuncOp func);
   LogicalResult convertBlock(Block &bb, bool ignoreArguments);
 
   llvm::Constant *getLLVMConstant(llvm::Type *llvmType, Attribute attr,
                                   Location loc);
 
   /// Original and translated module.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

if (failed(convertOperation(op, builder)))
       return failure();
   }
 
   return success();
 }
 
 /// Create named global variables that correspond to llvm.mlir.global
 /// definitions.
-void ModuleTranslation::convertGlobals() {
+LogicalResult ModuleTranslation::convertGlobals() {
   for (auto op : getModuleBody(mlirModule).getOps<LLVM::GlobalOp>()) {
     llvm::Type *type = op.getType().getUnderlyingType();
     llvm::Constant *cst = llvm::UndefValue::get(type);
     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 {
-        cst = getLLVMConstant(type, op.getValueOrNull(), op.getLoc());
+        if (!(cst = getLLVMConstant(type, op.getValueOrNull(), op.getLoc())))
+          return failure();
       }
     } else if (Block *initializer = op.getInitializerBlock()) {
       llvm::IRBuilder<> builder(llvmModule->getContext());
       for (auto &op : initializer->without_terminator()) {
         if (failed(convertOperation(op, builder)) ||
-            !isa<llvm::Constant>(valueMapping.lookup(op.getResult(0)))) {
-          emitError(op.getLoc(), "unemittable constant value");
-          return;
-        }
+            !isa<llvm::Constant>(valueMapping.lookup(op.getResult(0))))
+          return emitError(op.getLoc(), "unemittable constant value");
       }
       ReturnOp ret = cast<ReturnOp>(initializer->getTerminator());
       cst = cast<llvm::Constant>(valueMapping.lookup(ret.getOperand(0)));
     }
 
     auto linkage = convertLinkageToLLVM(op.linkage());
     bool anyExternalLinkage =
         (linkage == llvm::GlobalVariable::ExternalLinkage ||
          linkage == llvm::GlobalVariable::ExternalWeakLinkage);
     auto addrSpace = op.addr_space().getLimitedValue();
     auto *var = new llvm::GlobalVariable(
         *llvmModule, type, op.constant(), linkage,
         anyExternalLinkage ? nullptr : cst, op.sym_name(),
         /*InsertBefore=*/nullptr, llvm::GlobalValue::NotThreadLocal, addrSpace);
 
     globalsMapping.try_emplace(op, var);
   }
+
+  return success();
 }
 
 /// Get the SSA value passed to the current block from the terminator operation
 /// of its predecessor.
 static Value getPHISourceValue(Block *current, Block *pred,
                                unsigned numArguments, unsigned index) {
   auto &terminator = *pred->getTerminator();
   if (isa<LLVM::BrOp>(terminator)) {

mlir/test/Target/llvmir-invalid.mlir

 // RUN: mlir-translate -verify-diagnostics -split-input-file -mlir-to-llvmir %s
 
 // expected-error @+1 {{unsupported module-level operation}}
 func @foo() {
   llvm.return
 }
 
 // -----
 
 llvm.func @no_nested_struct() -> !llvm<"[2 x [2 x [2 x {i32}]]]"> {
   // expected-error @+1 {{struct types are not supported in constants}}
   %0 = llvm.mlir.constant(dense<[[[1, 2], [3, 4]], [[42, 43], [44, 45]]]> : tensor<2x2x2xi32>) : !llvm<"[2 x [2 x [2 x {i32}]]]">
   llvm.return %0 : !llvm<"[2 x [2 x [2 x {i32}]]]">
 }
+
+// -----
+
+// expected-error @+1 {{unsupported constant value}}
+llvm.mlir.global internal constant @test([2.5, 7.4]) : !llvm<"[2 x double]">