[Mlir-commits] [mlir] [mlir][func] Account for named attributes without a dialect prefix in `FuncToLLVM` lowering pass (PR #182987)

[Ayokunle Amodu]

================
@@ -87,12 +87,3 @@ func.func @multiple_arg_attr_multiple_res_attr(%arg0: memref<f32> {test.argZero
   %2 = arith.constant 2 : i32
   return %0, %1, %2 : f32, memref<i32>, i32
 }
-
-// CHECK: llvm.func @drop_linkage_attr() -> (!llvm.struct{{.*}} {test.returnOne})
-// CHECK-LABEL: llvm.func @_mlir_ciface_drop_linkage_attr
-// CHECK-SAME: !llvm.ptr
-// CHECK-NOT: llvm.linkage
-func.func @drop_linkage_attr() -> (memref<f32> {test.returnOne}) attributes { llvm.linkage = #llvm.linkage<external> } {
-  %0 = memref.alloc() : memref<f32>
-  return %0 : memref<f32>
-}
----------------
ayokunle321 wrote:

Because `getAttributeNames()` returns the raw attribute names. So if `linkage` is seen, it gets removed, but `llvm.linkage` would not.

I think why they had it hardcoded those particular names in the first place is because the build function already handles them. 

If you look at the [`convertFuncOpToLLVMFuncOp`](https://github.com/llvm/llvm-project/blob/59ba10b9d38a27e6783a64f21c0a4f56bccae126/mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp#L323), it basically initializes  all the attributes needed for `LLVMFuncOp::create` before the filter. 

For instance, if we had `llvm.passthrough`, which is ODS-defined , we would want this to remain in the attribute list and not get filtered out. The reason why it's fine for `llvm.linkage` to get filtered out is because it's meant to be handled by the build: 

```cpp
void LLVMFuncOp::build(OpBuilder &builder, OperationState &result,
                       StringRef name, Type type, LLVM::Linkage linkage,
                       bool dsoLocal, CConv cconv, SymbolRefAttr comdat,
                       ArrayRef<NamedAttribute> attrs,
                       ArrayRef<DictionaryAttr> argAttrs,
                       std::optional<uint64_t> functionEntryCount) {
  result.addRegion();
  result.addAttribute(SymbolTable::getSymbolAttrName(),
                      builder.getStringAttr(name));
  result.addAttribute(getFunctionTypeAttrName(result.name),
                      TypeAttr::get(type));
  result.addAttribute(getLinkageAttrName(result.name),
                      LinkageAttr::get(builder.getContext(), linkage));
  result.addAttribute(getCConvAttrName(result.name),
                      CConvAttr::get(builder.getContext(), cconv));
  result.attributes.append(attrs.begin(), attrs.end());
  
  if (dsoLocal)
    result.addAttribute(getDsoLocalAttrName(result.name),
                        builder.getUnitAttr());
  if (comdat)
    result.addAttribute(getComdatAttrName(result.name), comdat);
  if (functionEntryCount)
    result.addAttribute(getFunctionEntryCountAttrName(result.name),
                        builder.getI64IntegerAttr(functionEntryCount.value()));
  if (argAttrs.empty())
    return;
```

So essentially, I'm thinking we still need the hardcoded filter as well as the ODS one. Because we do not want to see an attribute list with either:

sym_name
function_type
linkage
CConv
linkage

or 

sym_name
function_type
linkage
CConv
llvm.linkage

but 

sym_name
function_type
linkage
CConv
llvm.passthrough

would be fine.

Thoughts?

https://github.com/llvm/llvm-project/pull/182987