[CIR] Better handling of `void` function return #128089

dkolsen-pgi · 2025-02-20T23:38:02Z

Upstream llvm/clangir#1249 "Remove return !cir.void from IR and textual representation"

C/C++ functions returning void had an explicit !cir.void return type while not having any returned value, which was breaking MLIR invariants when the CIR dialect is used in a greater context, for example with the inliner.

Now a C/C++ function returning void has no return type and no return values, which satisfies the MLIR invariant about the same number of return types and returned values.

Upstream llvm/clangir#1249 "Remove return !cir.void from IR and textual representation" C/C++ functions returning `void` had an explicit `!cir.void` return type while not having any returned value, which was breaking MLIR invariants when the CIR dialect is used in a greater context, for example with the inliner. Now a C/C++ function returning `void` has no return type and no return values, which satisfies the MLIR invariant about the same number of return types and returned values.

llvmbot · 2025-02-20T23:38:34Z

@llvm/pr-subscribers-clangir

@llvm/pr-subscribers-clang

Author: David Olsen (dkolsen-pgi)

Changes

Upstream llvm/clangir#1249 "Remove return !cir.void from IR and textual representation"

C/C++ functions returning void had an explicit !cir.void return type while not having any returned value, which was breaking MLIR invariants when the CIR dialect is used in a greater context, for example with the inliner.

Now a C/C++ function returning void has no return type and no return values, which satisfies the MLIR invariant about the same number of return types and returned values.

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

6 Files Affected:

(modified) clang/include/clang/CIR/Dialect/IR/CIRTypes.td (+17-6)
(modified) clang/lib/CIR/CodeGen/CIRGenTypes.cpp (+1-1)
(modified) clang/lib/CIR/Dialect/IR/CIRDialect.cpp (+5)
(modified) clang/lib/CIR/Dialect/IR/CIRTypes.cpp (+83-12)
(modified) clang/test/CIR/func-simple.cpp (+2-2)
(modified) clang/test/CIR/global-var-simple.cpp (+2-2)

diff --git a/clang/include/clang/CIR/Dialect/IR/CIRTypes.td b/clang/include/clang/CIR/Dialect/IR/CIRTypes.td
index a32fb3c801114..ad0935e6ea1e3 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIRTypes.td
+++ b/clang/include/clang/CIR/Dialect/IR/CIRTypes.td
@@ -273,29 +273,36 @@ def CIR_PointerType : CIR_Type<"Pointer", "ptr",
 def CIR_FuncType : CIR_Type<"Func", "func"> {
   let summary = "CIR function type";
   let description = [{
-    The `!cir.func` is a function type. It consists of a single return type, a
-    list of parameter types and can optionally be variadic.
+    The `!cir.func` is a function type. It consists of an optional return type,
+    a list of parameter types and can optionally be variadic.
 
     Example:
 
     ```mlir
+    !cir.func<()>
     !cir.func<!bool ()>
+    !cir.func<(!s8i, !s8i)>
     !cir.func<!s32i (!s8i, !s8i)>
     !cir.func<!s32i (!s32i, ...)>
     ```
   }];
 
   let parameters = (ins ArrayRefParameter<"mlir::Type">:$inputs,
-                        "mlir::Type":$returnType, "bool":$varArg);
+                        "mlir::Type":$optionalReturnType, "bool":$varArg);
+  // Use a custom parser to handle the optional return and argument types
+  // without an optional anchor.
   let assemblyFormat = [{
-    `<` $returnType ` ` `(` custom<FuncTypeArgs>($inputs, $varArg) `>`
+    `<` custom<FuncType>($optionalReturnType, $inputs, $varArg) `>`
   }];
 
   let builders = [
+    // Construct with an actual return type or explicit !cir.void
     TypeBuilderWithInferredContext<(ins
       "llvm::ArrayRef<mlir::Type>":$inputs, "mlir::Type":$returnType,
       CArg<"bool", "false">:$isVarArg), [{
-      return $_get(returnType.getContext(), inputs, returnType, isVarArg);
+        return $_get(returnType.getContext(), inputs,
+                     mlir::isa<cir::VoidType>(returnType) ? nullptr : returnType,
+                     isVarArg);
     }]>
   ];
 
@@ -309,11 +316,15 @@ def CIR_FuncType : CIR_Type<"Func", "func"> {
     /// Returns the number of arguments to the function.
     unsigned getNumInputs() const { return getInputs().size(); }
 
+    /// Returns the result type of the function as an actual return type or
+    /// explicit !cir.void
+    mlir::Type getReturnType() const;
+
     /// Returns the result type of the function as an ArrayRef, enabling better
     /// integration with generic MLIR utilities.
     llvm::ArrayRef<mlir::Type> getReturnTypes() const;
 
-    /// Returns whether the function is returns void.
+    /// Returns whether the function returns void.
     bool isVoid() const;
 
     /// Returns a clone of this function type with the given argument
diff --git a/clang/lib/CIR/CodeGen/CIRGenTypes.cpp b/clang/lib/CIR/CodeGen/CIRGenTypes.cpp
index 551b43ef121b3..72ace7520e087 100644
--- a/clang/lib/CIR/CodeGen/CIRGenTypes.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenTypes.cpp
@@ -60,7 +60,7 @@ bool CIRGenTypes::isFuncTypeConvertible(const FunctionType *ft) {
 mlir::Type CIRGenTypes::convertFunctionTypeInternal(QualType qft) {
   assert(qft.isCanonical());
   const FunctionType *ft = cast<FunctionType>(qft.getTypePtr());
-  // First, check whether we can build the full fucntion type. If the function
+  // First, check whether we can build the full function type. If the function
   // type depends on an incomplete type (e.g. a struct or enum), we cannot lower
   // the function type.
   if (!isFuncTypeConvertible(ft)) {
diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
index 10ad7fb4e6542..3c3b03ccf1b9a 100644
--- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
+++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
@@ -399,6 +399,11 @@ LogicalResult cir::FuncOp::verifyType() {
   if (!isa<cir::FuncType>(type))
     return emitOpError("requires '" + getFunctionTypeAttrName().str() +
                        "' attribute of function type");
+  if (auto rt = type.getReturnTypes();
+      !rt.empty() && mlir::isa<cir::VoidType>(rt.front()))
+    return emitOpError("The return type for a function returning void should "
+                       "be empty instead of an explicit !cir.void");
+
   return success();
 }
 
diff --git a/clang/lib/CIR/Dialect/IR/CIRTypes.cpp b/clang/lib/CIR/Dialect/IR/CIRTypes.cpp
index 48be11ba4e243..ee7039def1cda 100644
--- a/clang/lib/CIR/Dialect/IR/CIRTypes.cpp
+++ b/clang/lib/CIR/Dialect/IR/CIRTypes.cpp
@@ -16,15 +16,19 @@
 #include "clang/CIR/Dialect/IR/CIRDialect.h"
 #include "llvm/ADT/TypeSwitch.h"
 
+#include <cassert>
+
 //===----------------------------------------------------------------------===//
 // CIR Custom Parser/Printer Signatures
 //===----------------------------------------------------------------------===//
 
-static mlir::ParseResult
-parseFuncTypeArgs(mlir::AsmParser &p, llvm::SmallVector<mlir::Type> &params,
-                  bool &isVarArg);
-static void printFuncTypeArgs(mlir::AsmPrinter &p,
-                              mlir::ArrayRef<mlir::Type> params, bool isVarArg);
+static mlir::ParseResult parseFuncType(mlir::AsmParser &p,
+                                       mlir::Type &optionalReturnTypes,
+                                       llvm::SmallVector<mlir::Type> &params,
+                                       bool &isVarArg);
+
+static void printFuncType(mlir::AsmPrinter &p, mlir::Type optionalReturnTypes,
+                          mlir::ArrayRef<mlir::Type> params, bool isVarArg);
 
 //===----------------------------------------------------------------------===//
 // Get autogenerated stuff
@@ -331,9 +335,38 @@ FuncType FuncType::clone(TypeRange inputs, TypeRange results) const {
   return get(llvm::to_vector(inputs), results[0], isVarArg());
 }
 
-mlir::ParseResult parseFuncTypeArgs(mlir::AsmParser &p,
-                                    llvm::SmallVector<mlir::Type> &params,
-                                    bool &isVarArg) {
+// A special parser is needed for function returning void to handle the missing
+// type.
+static mlir::ParseResult parseFuncTypeReturn(mlir::AsmParser &p,
+                                             mlir::Type &optionalReturnType) {
+  if (succeeded(p.parseOptionalLParen())) {
+    // If we have already a '(', the function has no return type
+    optionalReturnType = {};
+    return mlir::success();
+  }
+  mlir::Type type;
+  if (p.parseType(type))
+    return mlir::failure();
+  if (isa<cir::VoidType>(type))
+    // An explicit !cir.void means also no return type.
+    optionalReturnType = {};
+  else
+    // Otherwise use the actual type.
+    optionalReturnType = type;
+  return p.parseLParen();
+}
+
+// A special pretty-printer for function returning or not a result.
+static void printFuncTypeReturn(mlir::AsmPrinter &p,
+                                mlir::Type optionalReturnType) {
+  if (optionalReturnType)
+    p << optionalReturnType << ' ';
+  p << '(';
+}
+
+static mlir::ParseResult
+parseFuncTypeArgs(mlir::AsmParser &p, llvm::SmallVector<mlir::Type> &params,
+                  bool &isVarArg) {
   isVarArg = false;
   // `(` `)`
   if (succeeded(p.parseOptionalRParen()))
@@ -363,8 +396,9 @@ mlir::ParseResult parseFuncTypeArgs(mlir::AsmParser &p,
   return p.parseRParen();
 }
 
-void printFuncTypeArgs(mlir::AsmPrinter &p, mlir::ArrayRef<mlir::Type> params,
-                       bool isVarArg) {
+static void printFuncTypeArgs(mlir::AsmPrinter &p,
+                              mlir::ArrayRef<mlir::Type> params,
+                              bool isVarArg) {
   llvm::interleaveComma(params, p,
                         [&p](mlir::Type type) { p.printType(type); });
   if (isVarArg) {
@@ -375,11 +409,48 @@ void printFuncTypeArgs(mlir::AsmPrinter &p, mlir::ArrayRef<mlir::Type> params,
   p << ')';
 }
 
+// Use a custom parser to handle the optional return and argument types without
+// an optional anchor.
+static mlir::ParseResult parseFuncType(mlir::AsmParser &p,
+                                       mlir::Type &optionalReturnTypes,
+                                       llvm::SmallVector<mlir::Type> &params,
+                                       bool &isVarArg) {
+  if (failed(parseFuncTypeReturn(p, optionalReturnTypes)))
+    return failure();
+  return parseFuncTypeArgs(p, params, isVarArg);
+}
+
+static void printFuncType(mlir::AsmPrinter &p, mlir::Type optionalReturnTypes,
+                          mlir::ArrayRef<mlir::Type> params, bool isVarArg) {
+  printFuncTypeReturn(p, optionalReturnTypes);
+  printFuncTypeArgs(p, params, isVarArg);
+}
+
+// Return the actual return type or an explicit !cir.void if the function does
+// not return anything
+mlir::Type FuncType::getReturnType() const {
+  if (isVoid())
+    return cir::VoidType::get(getContext());
+  return static_cast<detail::FuncTypeStorage *>(getImpl())->optionalReturnType;
+}
+
+/// Returns the result type of the function as an ArrayRef, enabling better
+/// integration with generic MLIR utilities.
 llvm::ArrayRef<mlir::Type> FuncType::getReturnTypes() const {
-  return static_cast<detail::FuncTypeStorage *>(getImpl())->returnType;
+  if (isVoid())
+    return {};
+  return static_cast<detail::FuncTypeStorage *>(getImpl())->optionalReturnType;
 }
 
-bool FuncType::isVoid() const { return mlir::isa<VoidType>(getReturnType()); }
+// Whether the function returns void
+bool FuncType::isVoid() const {
+  auto rt =
+      static_cast<detail::FuncTypeStorage *>(getImpl())->optionalReturnType;
+  assert((!rt || !mlir::isa<cir::VoidType>(rt)) &&
+         "The return type for a function returning void should be empty "
+         "instead of a real !cir.void");
+  return !rt;
+}
 
 //===----------------------------------------------------------------------===//
 // PointerType Definitions
diff --git a/clang/test/CIR/func-simple.cpp b/clang/test/CIR/func-simple.cpp
index 10c49bc506c87..f5d8b2225c7e4 100644
--- a/clang/test/CIR/func-simple.cpp
+++ b/clang/test/CIR/func-simple.cpp
@@ -2,12 +2,12 @@
 // RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir %s -o -  | FileCheck %s
 
 void empty() { }
-// CHECK: cir.func @empty() -> !cir.void {
+// CHECK: cir.func @empty() {
 // CHECK:   cir.return
 // CHECK: }
 
 void voidret() { return; }
-// CHECK: cir.func @voidret() -> !cir.void {
+// CHECK: cir.func @voidret() {
 // CHECK:   cir.return
 // CHECK: }
 
diff --git a/clang/test/CIR/global-var-simple.cpp b/clang/test/CIR/global-var-simple.cpp
index 237070a5b7564..cb3e915a6fa5a 100644
--- a/clang/test/CIR/global-var-simple.cpp
+++ b/clang/test/CIR/global-var-simple.cpp
@@ -89,10 +89,10 @@ char **cpp;
 // CHECK: cir.global @cpp : !cir.ptr<!cir.ptr<!cir.int<s, 8>>>
 
 void (*fp)();
-// CHECK: cir.global @fp : !cir.ptr<!cir.func<!cir.void ()>>
+// CHECK: cir.global @fp : !cir.ptr<!cir.func<()>>
 
 int (*fpii)(int) = 0;
 // CHECK: cir.global @fpii = #cir.ptr<null> : !cir.ptr<!cir.func<!cir.int<s, 32> (!cir.int<s, 32>)>>
 
 void (*fpvar)(int, ...);
-// CHECK: cir.global @fpvar : !cir.ptr<!cir.func<!cir.void (!cir.int<s, 32>, ...)>>
+// CHECK: cir.global @fpvar : !cir.ptr<!cir.func<(!cir.int<s, 32>, ...)>>

dkolsen-pgi · 2025-02-20T23:39:40Z

@keryell Please review this, since it is your change. (I don't know why GitHub won't let me add you as a reviewer.)

erichkeane · 2025-02-21T00:18:49Z

clang/include/clang/CIR/Dialect/IR/CIRTypes.td

  }];

  let builders = [
+    // Construct with an actual return type or explicit !cir.void


Should this be 'implicit' void? It actually looks like this is just doing 'return type or nothing (if void)'.

Also, clang comments should end with a full-stop.

I agree. I am changing "explicit" to "implicit".

No, I really meant explicit here.
If you put an explicit !cir.void in the assembly text, it will be discarded and will be nowhere in the IR.
It is as if there was
This is for compatibility with pas ClangIR behavior.
Of course, since we are up-streaming it, do we need this backward compatibility? If not, it might breaks more stuff while rebasing ClangIR downstream... 🤔

Even in the incubator it should be fine to remove the compatibility given that I don't know any users loading/emitting old CIR

clang/include/clang/CIR/Dialect/IR/CIRTypes.td

erichkeane · 2025-02-21T00:20:09Z

clang/lib/CIR/Dialect/IR/CIRTypes.cpp

+                                mlir::Type optionalReturnType) {
+  if (optionalReturnType)
+    p << optionalReturnType << ' ';
+  p << '(';


It seems a little weird that THIS function is the one that prints the open paren? Shouldn't whoever calls this print it?

One could argue that since parseFuncTypeReturn parses the opening ( of the parameter list, printFuncTypeReturn should match and print the opening ( of the parameter list.

But I'm not going to argue that. I agree with you. It makes more sense if the ( is output as part of printFuncTypeArgs instead of printFuncTypeReturn, even if the parse functions do it differently. I'll make that change.

erichkeane · 2025-02-21T00:21:07Z

clang/lib/CIR/Dialect/IR/CIRTypes.cpp

+static mlir::ParseResult parseFuncTypeReturn(mlir::AsmParser &p,
+                                             mlir::Type &optionalReturnType) {
+  if (succeeded(p.parseOptionalLParen())) {
+    // If we have already a '(', the function has no return type


I'm perhaps a little confused as to what the format here is we're trying to parse. Why is just a ( mean empty return type?

Or is this supposed to be the 'rest' of the function type? In that case, perhaps we need a 'look-ahead' instead of a parse?

Yes this is a kind of look-ahead which is required when I introduced the change to remove the creation of an artificial !cir.void which used to have the consequence of breaking an MLIR invariant, number(return-types) == number(return-values).
A C type like char(int) is lowered and pretty-printed as !cir.func<!cir.int<s, 8>(!cir.int<s, 32>) while void(int) is lowered and pretty-printed as !cir.func<(!cir.int<s, 32>).
For the MLIR functions themselves, they are handled by the func MLIR standard dialect with a syntax like:

func.func @count(%x: i64) -> (i64, i64) // The same returning nothing: func.func @f(%x: i64)

with an empty type for returning "void".
An alternate design could be to have a new keyword like void understood by the parser to avoid the manual look ahead in the parser as the MLIR default parser knows how to discriminate with prefix keywords. Then void(int) would be lowered and pretty-printed as !cir.func<void (!cir.int<s, 32>).

erichkeane · 2025-02-21T00:21:41Z

clang/lib/CIR/Dialect/IR/CIRTypes.cpp

+  if (p.parseType(type))
+    return mlir::failure();
+  if (isa<cir::VoidType>(type))
+    // An explicit !cir.void means also no return type.


Typically a comment is enough to cause us to not do the omit braces rule here, so I suggest putting curleys around this if and else.

We could put the comment before the if otherwise.

erichkeane · 2025-02-21T00:24:01Z

clang/lib/CIR/Dialect/IR/CIRTypes.cpp

+
+static mlir::ParseResult
+parseFuncTypeArgs(mlir::AsmParser &p, llvm::SmallVector<mlir::Type> &params,
+                  bool &isVarArg) {
  isVarArg = false;
  // `(` `)`
  if (succeeded(p.parseOptionalRParen()))


I see what is happening here now, not sure I like it? IMO, the parse-args function should be responsible for 'taking' BOTH of the parens, everything else should do a 'look-ahead' type parsing if necessary. THOUGH I see the latter doesn't seem to exist in mlir::AsmParser? Is this a case of us 'using it wrong'? What do other parsers do here?

I am not familiar with mlir::AsmParser, but looking through its definition just now I don't see anyway to peek at the next token or do lookahead or do a trial parse without advancing. I agree that parseFuncTypeArgs should handle both the ( and the ). But I don't see a way to do that. I don't have experience with other MLIR parsers, so I don't know what they do. Unless someone more knowledgeable pipes up, I don't see a way to change this.

@lanza @bcardosolopes Anyone have any ideas here? This parsing is REALLY weird, but we still need a way to know that we're 'done'.

The other alternative is to change how these are printed/read (so it is something like: (returnType)(Arg1, Arg2) with whatever delimiters make sense), but I'm not sure that is something that can be done sensibly.

I think this is the way the parsers are implemented to chain things ahead.
See again my comment with void keyword.
At the end it is a trade-off between implementator comfort and code beauty vs the concrete assembly syntax exposed to the end user.

You can technically do it the other way around as mlir function types are structured and have !cir.func<(args) -> ret> or !cir.func<(args)> in case of void. This can be parsed without lookahead using parseOptionalArrow.

@bcardosolopes @lanza ^

The only question is what the 'void' type looks like? Could/should we still use the void as the return type in the textual representation, or alternatively just omit entirely? So:

!cir.func<(args) -> void>
vs:
!cir.func<(args)>

That way we do parse arguments, then if arrow, parse return type, else is void.

The 'trailing return type' syntax is nice as it is consistent with the C++ language.

-> void would omitted in the trailing return type syntax. A function type with a void return in C or C++ would be encoded as !cir.func<(args)>. The "if arrow, parse return type, else is void" that you suggested is exactly how it is done. I already have a PR for this change in the incubator waiting for reviews and approval, llvm/clangir#1391

Got it, thanks for the update! I was trying to judge opinions here, but the incubator review is sufficient to make sure those opinions are heard.

I like the idea of the !cir.func<(args) -> ret> format

+1

Fix some comments and some formatting. Change which function prints the opening `(` of the parameter list of a function type. No changes in behavior.

xlauko · 2025-02-21T15:03:07Z

clang/lib/CIR/Dialect/IR/CIRTypes.cpp

+mlir::Type FuncType::getReturnType() const {
+  if (isVoid())
+    return cir::VoidType::get(getContext());
+  return static_cast<detail::FuncTypeStorage *>(getImpl())->optionalReturnType;


Suggested change

return static_cast<detail::FuncTypeStorage *>(getImpl())->optionalReturnType;

return getOptionalReturnType();

xlauko · 2025-02-21T15:03:22Z

clang/lib/CIR/Dialect/IR/CIRTypes.cpp

-  return static_cast<detail::FuncTypeStorage *>(getImpl())->returnType;
+  if (isVoid())
+    return {};
+  return static_cast<detail::FuncTypeStorage *>(getImpl())->optionalReturnType;


Suggested change

return static_cast<detail::FuncTypeStorage *>(getImpl())->optionalReturnType;

return getImpl()->optionalReturnType;

Also to mirror getReturnType shouldn't this return cir::VoidType::get(getContext()) in void case?

No because it returns the real return types (plural) that any real MLIR user is waiting for: the actual return types, which in the case of C/C++ is 0 or 1 type.
The whole point of this PR is to fix this bug and not to return some synthetic !cir.void which was breaking the neighborhood.

xlauko · 2025-02-21T15:03:37Z

clang/lib/CIR/Dialect/IR/CIRTypes.cpp

+// Whether the function returns void
+bool FuncType::isVoid() const {
+  auto rt =
+      static_cast<detail::FuncTypeStorage *>(getImpl())->optionalReturnType;


Suggested change

static_cast<detail::FuncTypeStorage *>(getImpl())->optionalReturnType;

getOptionalReturnType();

xlauko · 2025-02-21T16:02:08Z

clang/test/CIR/func-simple.cpp

@@ -2,12 +2,12 @@
 // RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir %s -o -  | FileCheck %s


I presume clang cannot load cir files yet? So nothing is really testing parsers?

It's definitely unfortunate that we're adding this parsing code but don't have any tests for it. I'm working on a PR to bring cir-opt over so we can do that.

xlauko · 2025-02-21T17:50:02Z

clang/lib/CIR/Dialect/IR/CIRTypes.cpp

+// Use a custom parser to handle the optional return and argument types without
+// an optional anchor.
+static mlir::ParseResult parseFuncType(mlir::AsmParser &p,
+                                       mlir::Type &optionalReturnTypes,


Suggested change

mlir::Type &optionalReturnTypes,

mlir::Type &optionalReturnType,

xlauko · 2025-02-21T17:50:29Z

clang/lib/CIR/Dialect/IR/CIRTypes.cpp

+  return parseFuncTypeArgs(p, params, isVarArg);
+}
+
+static void printFuncType(mlir::AsmPrinter &p, mlir::Type optionalReturnTypes,


Suggested change

static void printFuncType(mlir::AsmPrinter &p, mlir::Type optionalReturnTypes,

static void printFuncType(mlir::AsmPrinter &p, mlir::Type optionalReturnType,

xlauko · 2025-02-21T17:51:56Z

clang/lib/CIR/Dialect/IR/CIRTypes.cpp

+                                       mlir::Type &optionalReturnTypes,
+                                       llvm::SmallVector<mlir::Type> &params,
+                                       bool &isVarArg) {
+  if (failed(parseFuncTypeReturn(p, optionalReturnTypes)))


Suggested change

if (failed(parseFuncTypeReturn(p, optionalReturnTypes)))

if (failed(parseFuncTypeReturn(p, optionalReturnType)))

xlauko · 2025-02-21T17:52:07Z

clang/lib/CIR/Dialect/IR/CIRTypes.cpp

+
+static void printFuncType(mlir::AsmPrinter &p, mlir::Type optionalReturnTypes,
+                          mlir::ArrayRef<mlir::Type> params, bool isVarArg) {
+  printFuncTypeReturn(p, optionalReturnTypes);


Suggested change

printFuncTypeReturn(p, optionalReturnTypes);

printFuncTypeReturn(p, optionalReturnType);

keryell

Sorry for being late to the party!

keryell · 2025-02-21T19:43:58Z

clang/lib/CIR/Dialect/IR/CIRTypes.cpp

+  if (p.parseType(type))
+    return mlir::failure();
+  if (isa<cir::VoidType>(type))
+    // An explicit !cir.void means also no return type.


We could put the comment before the if otherwise.

keryell · 2025-02-21T21:16:57Z

clang/lib/CIR/Dialect/IR/CIRTypes.cpp

+static mlir::ParseResult parseFuncTypeReturn(mlir::AsmParser &p,
+                                             mlir::Type &optionalReturnType) {
+  if (succeeded(p.parseOptionalLParen())) {
+    // If we have already a '(', the function has no return type


Yes this is a kind of look-ahead which is required when I introduced the change to remove the creation of an artificial !cir.void which used to have the consequence of breaking an MLIR invariant, number(return-types) == number(return-values).
A C type like char(int) is lowered and pretty-printed as !cir.func<!cir.int<s, 8>(!cir.int<s, 32>) while void(int) is lowered and pretty-printed as !cir.func<(!cir.int<s, 32>).
For the MLIR functions themselves, they are handled by the func MLIR standard dialect with a syntax like:

func.func @count(%x: i64) -> (i64, i64) // The same returning nothing: func.func @f(%x: i64)

with an empty type for returning "void".
An alternate design could be to have a new keyword like void understood by the parser to avoid the manual look ahead in the parser as the MLIR default parser knows how to discriminate with prefix keywords. Then void(int) would be lowered and pretty-printed as !cir.func<void (!cir.int<s, 32>).

keryell · 2025-02-21T21:21:03Z

clang/lib/CIR/Dialect/IR/CIRTypes.cpp

+
+static mlir::ParseResult
+parseFuncTypeArgs(mlir::AsmParser &p, llvm::SmallVector<mlir::Type> &params,
+                  bool &isVarArg) {
  isVarArg = false;
  // `(` `)`
  if (succeeded(p.parseOptionalRParen()))


I think this is the way the parsers are implemented to chain things ahead.
See again my comment with void keyword.
At the end it is a trade-off between implementator comfort and code beauty vs the concrete assembly syntax exposed to the end user.

keryell · 2025-02-21T21:28:35Z

clang/lib/CIR/Dialect/IR/CIRTypes.cpp

-  return static_cast<detail::FuncTypeStorage *>(getImpl())->returnType;
+  if (isVoid())
+    return {};
+  return static_cast<detail::FuncTypeStorage *>(getImpl())->optionalReturnType;


No because it returns the real return types (plural) that any real MLIR user is waiting for: the actual return types, which in the case of C/C++ is 0 or 1 type.
The whole point of this PR is to fix this bug and not to return some synthetic !cir.void which was breaking the neighborhood.

keryell · 2025-02-21T21:37:14Z

clang/include/clang/CIR/Dialect/IR/CIRTypes.td

  }];

  let builders = [
+    // Construct with an actual return type or explicit !cir.void


No, I really meant explicit here.
If you put an explicit !cir.void in the assembly text, it will be discarded and will be nowhere in the IR.
It is as if there was
This is for compatibility with pas ClangIR behavior.
Of course, since we are up-streaming it, do we need this backward compatibility? If not, it might breaks more stuff while rebasing ClangIR downstream... 🤔

dkolsen-pgi · 2025-02-22T20:09:30Z

I like the idea of changing the assembly format for function types from !cir.func<!returnType (!argType)> to !cir.func<(!argType) -> !returnType>. That is

Easier to parse.
Consistent with function types in other MLIR dialects.
Consistent with the assembly format for function definitions in ClangIR.

Making a change like this really needs to happen in the incubator first, not as part of upstreaming. So my current plan is to

abandon this PR,
create a new PR in the incubator to make this change (including fixing lots of tests),
and then create a new upstream PR with the new behavior.

bcardosolopes · 2025-02-24T18:51:35Z

Thanks for the update @dkolsen-pgi, reviewing the incubator change today

dkolsen-pgi · 2025-02-25T19:38:22Z

Abandoning this PR. I will redo it soon with the new assembly format for function types.

dkolsen-pgi requested review from erichkeane and andykaylor February 20, 2025 23:38

dkolsen-pgi requested review from lanza and bcardosolopes as code owners February 20, 2025 23:38

llvmbot added clang Clang issues not falling into any other category ClangIR Anything related to the ClangIR project labels Feb 20, 2025

erichkeane reviewed Feb 21, 2025

View reviewed changes

Followup to void return functions in CIR

4b25f4d

Fix some comments and some formatting. Change which function prints the opening `(` of the parameter list of a function type. No changes in behavior.

xlauko reviewed Feb 21, 2025

View reviewed changes

keryell reviewed Feb 21, 2025

View reviewed changes

dkolsen-pgi closed this Feb 25, 2025

	return static_cast<detail::FuncTypeStorage *>(getImpl())->optionalReturnType;
	return getOptionalReturnType();

	return static_cast<detail::FuncTypeStorage *>(getImpl())->optionalReturnType;
	return getImpl()->optionalReturnType;

	static_cast<detail::FuncTypeStorage *>(getImpl())->optionalReturnType;
	getOptionalReturnType();

		@@ -2,12 +2,12 @@
		// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir %s -o - \| FileCheck %s

	mlir::Type &optionalReturnTypes,
	mlir::Type &optionalReturnType,

	static void printFuncType(mlir::AsmPrinter &p, mlir::Type optionalReturnTypes,
	static void printFuncType(mlir::AsmPrinter &p, mlir::Type optionalReturnType,

	if (failed(parseFuncTypeReturn(p, optionalReturnTypes)))
	if (failed(parseFuncTypeReturn(p, optionalReturnType)))

	printFuncTypeReturn(p, optionalReturnTypes);
	printFuncTypeReturn(p, optionalReturnType);

[CIR] Better handling of void function return #128089

[CIR] Better handling of void function return #128089

Uh oh!

Conversation

dkolsen-pgi commented Feb 20, 2025

Uh oh!

llvmbot commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dkolsen-pgi commented Feb 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keryell Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xlauko Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

[CIR] Better handling of `void` function return #128089

[CIR] Better handling of `void` function return #128089

llvmbot commented Feb 20, 2025 •

edited

Loading

keryell Feb 21, 2025 •

edited

Loading

xlauko Feb 21, 2025 •

edited

Loading

keryell Feb 21, 2025 •

edited

Loading