[mlir][FuncToLLVM] Fix arg attr memref interaction

[Dinistro]

Summary

This commit ensures that argument attributes are dropped from types
that are expanded to multiple function arguments. Previously, they
were attached to all arguments belonging to the memref, which
resulted in illegal LLVMIR, e.g., noalias on integers.

[gysit]

mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp

367–368

Could this be done in the place where the argument (=memref) is unpacked to multiple arguments? It seems like this logic is very argument type specific.

[Dinistro]

mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp

367–368

Unfortunately, the SignatureConversion cannot do that for us. It might make sense to differentiate on the original type and only perform this change when it is a memref type.

[Dinistro]

Changed the handling to drop all argument attributes in the case of a type expanding to multiple arguments

[gysit]

LGTM!

[ftynse]

The diff description no longer matches the change.

[Dinistro]

In D142212#4072666, @ftynse wrote:

The diff description no longer matches the change.

Good catch! I changed it accordingly.

mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp

if (ArrayAttr argAttrDicts = funcOp.getAllArgAttrs()) {
             convertedAttrs.push_back(rewriter.getNamedAttr(
                 LLVM::LLVMDialect::getInAllocaAttrName(), convert(attr)));
           } else {
             convertedAttrs.push_back(attr);
           }
         }
         auto mapping = result.getInputMapping(i);
         assert(mapping && "unexpected deletion of function argument");
+        // Only attach the new argument attributes if there is a one-to-one
+        // mapping from old to new types. Otherwise, attributes might be
+        // attached to types that they do not support.
+        if (mapping->size == 1) {
+          newArgAttrs[mapping->inputNo] =
+              DictionaryAttr::get(rewriter.getContext(), convertedAttrs);
+          continue;
+        }
+        // TODO: Implement custom handling for types that expand to multiple
+        // function arguments.
         for (size_t j = 0; j < mapping->size; ++j)
           newArgAttrs[mapping->inputNo + j] =
-              DictionaryAttr::get(rewriter.getContext(), convertedAttrs);
+              DictionaryAttr::get(rewriter.getContext(), {});
       }
       attributes.push_back(rewriter.getNamedAttr(
           funcOp.getArgAttrsAttrName(), rewriter.getArrayAttr(newArgAttrs)));
     }
 
     // Create an LLVM function, use external linkage by default until MLIR
     // functions have linkage.
     LLVM::Linkage linkage = LLVM::Linkage::External;

mlir/test/Conversion/FuncToLLVM/convert-argattrs.mlir

 // RUN: mlir-opt -convert-func-to-llvm %s | FileCheck %s
 
 // CHECK-LABEL: func @check_attributes
-// When expanding the memref to multiple arguments, argument attributes are replicated.
-// CHECK-COUNT-7: {dialect.a = true, dialect.b = 4 : i64}
-func.func @check_attributes(%static: memref<10x20xf32> {dialect.a = true, dialect.b = 4 : i64 }) {
+// CHECK-SAME: {dialect.a = true, dialect.b = 4 : i64}
+func.func @check_attributes(%int: i64 {dialect.a = true, dialect.b = 4 : i64 }) {
+  return
+}
+
+// CHECK-LABEL: func @check_memref
+// When expanding the memref to multiple arguments, argument attributes should be dropped entirely.
+// CHECK-NOT: {llvm.noalias}
+func.func @check_memref(%static: memref<10x20xf32> {llvm.noalias}) {
   return
 }
 
 // CHECK-LABEL: func @check_multiple
-// Make sure arguments attributes are attached to the right argument. We match
-// commas in the argument list for this purpose.
-// CHECK: %{{.*}}: !llvm{{.*}} {first.arg = true}, %{{.*}}: !llvm{{.*}} {first.arg = true}, %{{.*}}: i{{.*}} {first.arg = true},
-// CHECK-SAME: %{{.*}}: !llvm{{.*}} {second.arg = 42 : i32}, %{{.*}}: !llvm{{.*}} {second.arg = 42 : i32}, %{{.*}}: i{{.*}} {second.arg = 42 : i32})
-func.func @check_multiple(%first: memref<f32> {first.arg = true}, %second: memref<f32> {second.arg = 42 : i32}) {
+// CHECK-SAME: %{{.*}}: f32 {first.arg = true}, %{{.*}}: i64 {second.arg = 42 : i32}
+func.func @check_multiple(%first: f32 {first.arg = true}, %second: i64 {second.arg = 42 : i32}) {
   return
 }