[CIR] Start printing/parsing func 'attributes'

[erichkeane]

This patch adds a print and parse ability for the func to have MLIR-standard 'attributes' printed along side the standard function.

This patch also seeds the initial "disallowed" list so that we don't print things that we have custom printing for, AND will disallow them from being parsed. I believe this list to be complete, and it passes all tests.

This printing of attributes is necessary for testing some OpenACC things that putting into the normal func-printing seems unnecessary.

[llvmbot]

@llvm/pr-subscribers-clangir

@llvm/pr-subscribers-clang

Author: Erich Keane (erichkeane)

Changes

This patch adds a print and parse ability for the func to have MLIR-standard 'attributes' printed along side the standard function.

This patch also seeds the initial "disallowed" list so that we don't print things that we have custom printing for, AND will disallow them from being parsed. I believe this list to be complete, and it passes all tests.

This printing of attributes is necessary for testing some OpenACC things that putting into the normal func-printing seems unnecessary.

---

Full diff: https://github.com/llvm/llvm-project/pull/169674.diff

4 Files Affected:

• (modified) clang/include/clang/CIR/Dialect/IR/CIRTypes.td (+19)
• (modified) clang/lib/CIR/Dialect/IR/CIRDialect.cpp (+17)
• (modified) clang/test/CIR/IR/func.cir (+8)
• (added) clang/test/CIR/IR/invalid-func-attr.cir (+11)

diff --git a/clang/include/clang/CIR/Dialect/IR/CIRTypes.td b/clang/include/clang/CIR/Dialect/IR/CIRTypes.td
index 3e062add6633a..c0333305ad030 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIRTypes.td
+++ b/clang/include/clang/CIR/Dialect/IR/CIRTypes.td
@@ -509,6 +509,25 @@ def CIR_FuncType : CIR_Type<"Func", "func"> {
     /// Returns a clone of this function type with the given argument
     /// and result types.
     FuncType clone(mlir::TypeRange inputs, mlir::TypeRange results) const;
+
+    /// A list of mlir attributes that shouldn't appear in the generic
+    /// 'attributes' list, and instead are handled via other syntax.
+    static constexpr llvm::StringRef disallowedFromAttrList[] = {
+        "alias",
+        "builtin",
+        "comdat",
+        "coroutine",
+        "cxx_special_member",
+        "dso_local",
+        "function_type",
+        "global_ctor_priority",
+        "global_dtor_priority",
+        "global_visibility",
+        "inline_kind",
+        "lambda",
+        "linkage",
+        "no_proto",
+        "sym_visibility"};
   }];
 }
 
diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
index 6bf543cf794b7..5c1e4010c5469 100644
--- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
+++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
@@ -1824,6 +1824,20 @@ ParseResult cir::FuncOp::parse(OpAsmParser &parser, OperationState &state) {
       return failure();
   }
 
+  // Parse the rest of the attributes.
+  NamedAttrList parsedAttrs;
+  if (parser.parseOptionalAttrDictWithKeyword(parsedAttrs))
+    return failure();
+
+  for (StringRef disallowed : cir::FuncType::disallowedFromAttrList) {
+    if (parsedAttrs.get(disallowed))
+      return parser.emitError(loc, "attribute '")
+             << disallowed
+             << "' should not be specified in the explicit attribute list";
+  }
+
+  state.attributes.append(parsedAttrs);
+
   // Parse the optional function body.
   auto *body = state.addRegion();
   OptionalParseResult parseResult = parser.parseOptionalRegion(
@@ -1977,6 +1991,9 @@ void cir::FuncOp::print(OpAsmPrinter &p) {
     p << " inline(" << cir::stringifyInlineKind(inlineAttr.getValue()) << ")";
   }
 
+  function_interface_impl::printFunctionAttributes(
+      p, *this, cir::FuncType::disallowedFromAttrList);
+
   // Print the body if this is not an external function.
   Region &body = getOperation()->getRegion(0);
   if (!body.empty()) {
diff --git a/clang/test/CIR/IR/func.cir b/clang/test/CIR/IR/func.cir
index 10df27b7e168f..2b3ee24995f40 100644
--- a/clang/test/CIR/IR/func.cir
+++ b/clang/test/CIR/IR/func.cir
@@ -186,3 +186,11 @@ cir.func @Foo_move_assign() special_member<#cir.cxx_assign<!rec_Foo, move>> {
 // CHECK: cir.func @Foo_move_assign() special_member<#cir.cxx_assign<!rec_Foo, move>> {
 // CHECK:   cir.return
 // CHECK: }
+
+cir.func @has_attrs() attributes {foo, baz = 5, floof = "flop"} {
+  cir.return
+}
+
+// CHECK: cir.func @has_attrs() attributes {bz = 5, floof = "flop", foo} {
+// CHECK:   cir.return
+// CHECK: }
diff --git a/clang/test/CIR/IR/invalid-func-attr.cir b/clang/test/CIR/IR/invalid-func-attr.cir
new file mode 100644
index 0000000000000..aaaaba7a7bf6f
--- /dev/null
+++ b/clang/test/CIR/IR/invalid-func-attr.cir
@@ -0,0 +1,11 @@
+// RUN: cir-opt %s -verify-diagnostics
+
+module {
+  cir.func @l0() {
+    cir.return
+  }
+
+  cir.func @disallowedAttr() attributes {comdat} { // expected-error{{custom op 'cir.func' attribute 'comdat' should not be specified in the explicit attribute list}}
+    cir.return
+  }
+}

[erichkeane]

Hi all! I saw that func.func and a few others all do similar printing here, so it made sense to me that we print them too. I have an attribute for OpenACC that needs this, and isn't worth adding as a normal 'markup' (because it would also involve #including' OpenACC MLIR headers everywhere, and also, its a rare situation).

I know this is a significant change in direction, and assume attributes were omitted this way for some reason in the past, so I'd like @bcardosolopes in particular to take a look at this.

[github-actions]

🐧 Linux x64 Test Results

• 112190 tests passed
• 4078 tests skipped

[andykaylor]

This seems reasonable to me, though I'd like to hear what @bcardosolopes and @xlauko have to say about it.

[andykaylor]

I see that the other in-tree callers of printFunctionAttributes are using get<>AttrName to construct a list like this at the call site. In some cases these appear to be wrapper functions in the .td files that get a raw string, but in other cases I think they're generated from the op definition. I see that the generated code for cir::FuncOp has such functions. It also has this, which is suspiciously close to your list:

  static ::llvm::ArrayRef<::llvm::StringRef> getAttributeNames() {
    static ::llvm::StringRef attrNames[] = {
         ::llvm::StringRef("aliasee"),
         ::llvm::StringRef("arg_attrs"),
         ::llvm::StringRef("builtin"),
         ::llvm::StringRef("comdat"),
         ::llvm::StringRef("coroutine"),
         ::llvm::StringRef("cxx_special_member"),
         ::llvm::StringRef("dso_local"),
         ::llvm::StringRef("function_type"),
         ::llvm::StringRef("global_ctor_priority"),
         ::llvm::StringRef("global_dtor_priority"),
         ::llvm::StringRef("global_visibility"),
         ::llvm::StringRef("inline_kind"),
         ::llvm::StringRef("lambda"),
         ::llvm::StringRef("linkage"),
         ::llvm::StringRef("no_proto"),
         ::llvm::StringRef("res_attrs"),
         ::llvm::StringRef("sym_name"),
         ::llvm::StringRef("sym_visibility")
    };
    return ::llvm::ArrayRef(attrNames);
  }

[erichkeane]

OH! That IS shockingly close... Looks like I missed 3 :) Yes, that is EXACTLY what I want, I'll just use that.

(alias was picked up from our print method, so it seems that it is different spelling perhaps?).

Anyway, I'll replace it with this instead, thanks!

[erichkeane]

Pre-commit CI issue is this one: https://github.com/llvm/llvm-project/pull/169404/files#r2566277435

[xlauko]

Cool, this looks reasonable :) FYI I started cleaning this up in incubator: llvm/clangir#2028

Moving away from extra_attrs a putting them explicitly on the function. Eventually I hope we will be able to use assembly form direcly which will even for your case here be just attr-dict-with-keyword​ and the rest should happen automatically.

[xlauko]

I believe similarly to OpenACC we should put OpenCL to AttrDict.