[MLIR][LLVM] Add error if using incorrect attribute type for specifying LLVM linkage

[wsmoses]

Summary

Address post-commit review in https://reviews.llvm.org/D108524 to add appropriate diagnostics.

[mehdi_amini]

LG with comment!

mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp

269–276

In general we avoid isa->cast

linkage = funcOp->getAttr("llvm.linkage")
              .dyn_cast<mlir::LLVM::LinkageAttr>()
              .getLinkage();
if (!linkage) {
  funcOp->emitError() << "Contains llvm.linkage attribute not of type LLVM::LinkageAttr";
  return nullptr;
}

271

Seems above 80chars to me, can you double check this is clang-formatted?

mlir/test/Conversion/StandardToLLVM/convert-funcs.mlir

43

(Spurious change)

[bondhugula]

mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp

269–276

You are using the result of a dyn_cast unchecked here.

[wsmoses]

Address comments

[mehdi_amini]

mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp

269–276

Grahh you're right, I went too fast here.
The alternative aren't as enticing though:

auto linkageAttr = funcOp->getAttr("llvm.linkage").dyn_cast<mlir::LLVM::LinkageAttr>();
if (! linkageAttr) {
  funcOp->emitError() << "Contains llvm.linkage attribute not of type LLVM::LinkageAttr";
  return nullptr;
}
linkage = linkageAttr.getLinkage();

or:

if (auto linkageAttr = funcOp->getAttr("llvm.linkage").dyn_cast<mlir::LLVM::LinkageAttr>()) {
    linkage = linkageAttr.getLinkage();
} else {
  funcOp->emitError() << "Contains llvm.linkage attribute not of type LLVM::LinkageAttr";
  return nullptr;
}

269–276

I see that Billy already adopted the first one, LGTM

mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp

for (auto pair : llvm::enumerate(attributes)) {
         break;
       }
     }
 
     // Create an LLVM function, use external linkage by default until MLIR
     // functions have linkage.
     LLVM::Linkage linkage = LLVM::Linkage::External;
     if (funcOp->hasAttr("llvm.linkage")) {
-      linkage = funcOp->getAttr("llvm.linkage")
-                    .cast<mlir::LLVM::LinkageAttr>()
-                    .getLinkage();
+      auto attr =
+          funcOp->getAttr("llvm.linkage").dyn_cast<mlir::LLVM::LinkageAttr>();
+      if (!attr) {
+        funcOp->emitError()
+            << "Contains llvm.linkage attribute not of type LLVM::LinkageAttr";
+        return nullptr;
+      }
+      linkage = attr.getLinkage();
     }
     auto newFuncOp = rewriter.create<LLVM::LLVMFuncOp>(
         funcOp.getLoc(), funcOp.getName(), llvmType, linkage,
         /*dsoLocal*/ false, attributes);
     rewriter.inlineRegionBefore(funcOp.getBody(), newFuncOp.getBody(),
                                 newFuncOp.end());
     if (failed(rewriter.convertRegionTypes(&newFuncOp.getBody(), *typeConverter,
                                            &result)))

mlir/test/Conversion/StandardToLLVM/convert-funcs.mlir

-// RUN: mlir-opt -convert-std-to-llvm %s | FileCheck %s
+// RUN: mlir-opt -convert-std-to-llvm -split-input-file -verify-diagnostics %s | FileCheck %s
 
 //CHECK: llvm.func @second_order_arg(!llvm.ptr<func<void ()>>)
 func private @second_order_arg(%arg0 : () -> ())
 
 //CHECK: llvm.func @second_order_result() -> !llvm.ptr<func<void ()>>
 func private @second_order_result() -> (() -> ())
 
 //CHECK: llvm.func @second_order_multi_result() -> !llvm.struct<(ptr<func<i32 ()>>, ptr<func<i64 ()>>, ptr<func<f32 ()>>)>

 ^bb1(%bbarg: () -> ()):
 // CHECK-NEXT:  llvm.return %0 : !llvm.ptr<func<void ()>>
   return %bbarg : () -> ()
 }
 
 // CHECK-LABEL: llvm.func extern_weak @llvmlinkage(i32)
 func private @llvmlinkage(i32) attributes { "llvm.linkage" = #llvm.linkage<extern_weak> }
 
+
 // CHECK-LABEL: llvm.func @body(i32)
 func private @body(i32)
 
 // CHECK-LABEL: llvm.func @indirect_const_call
 // CHECK-SAME: (%[[ARG0:.*]]: i32) {
 func @indirect_const_call(%arg0: i32) {
 // CHECK-NEXT: %[[ADDR:.*]] = llvm.mlir.addressof @body : !llvm.ptr<func<void (i32)>>
   %0 = constant @body : (i32) -> ()
 // CHECK-NEXT:  llvm.call %[[ADDR]](%[[ARG0:.*]]) : (i32) -> ()
   call_indirect %0(%arg0) : (i32) -> ()
 // CHECK-NEXT:  llvm.return
   return
 }
 
 // CHECK-LABEL: llvm.func @indirect_call(%arg0: !llvm.ptr<func<i32 (f32)>>, %arg1: f32) -> i32 {
 func @indirect_call(%arg0: (f32) -> i32, %arg1: f32) -> i32 {
 // CHECK-NEXT:  %0 = llvm.call %arg0(%arg1) : (f32) -> i32
   %0 = call_indirect %arg0(%arg1) : (f32) -> i32
 // CHECK-NEXT:  llvm.return %0 : i32
   return %0 : i32
 }
 
+// -----
+
+func private @badllvmlinkage(i32) attributes { "llvm.linkage" = 3 : i64 } // expected-error {{Contains llvm.linkage attribute not of type LLVM::LinkageAttr}}