Skip to content

[CIR] Better handling of void function return #128089

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

[CIR] Better handling of void function return #128089

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
88e90df
[CIR] Better handling of `void` function return
dkolsen-pgi Feb 20, 2025
4b25f4d
Followup to void return functions in CIR
dkolsen-pgi Feb 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions clang/include/clang/CIR/Dialect/IR/CIRTypes.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 implicit !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);
}]>
];

Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/CIR/CodeGen/CIRGenTypes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down
5 changes: 5 additions & 0 deletions clang/lib/CIR/Dialect/IR/CIRDialect.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down
96 changes: 84 additions & 12 deletions clang/lib/CIR/Dialect/IR/CIRTypes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@keryell keryell Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>).

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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could put the comment before the if otherwise.

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 << ' ';
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bcardosolopes @lanza ^

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-> 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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

+1

Expand Down Expand Up @@ -363,8 +396,10 @@ 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) {
p << '(';
llvm::interleaveComma(params, p,
[&p](mlir::Type type) { p.printType(type); });
if (isVarArg) {
Expand All @@ -375,11 +410,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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
mlir::Type &optionalReturnTypes,
mlir::Type &optionalReturnType,

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return failure();
return parseFuncTypeArgs(p, params, isVarArg);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

mlir::ArrayRef<mlir::Type> params, bool isVarArg) {
printFuncTypeReturn(p, optionalReturnTypes);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/// 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;
Copy link
Contributor

@xlauko xlauko Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Expand Down
4 changes: 2 additions & 2 deletions clang/test/CIR/func-simple.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@
// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir %s -o - | FileCheck %s
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#128254


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: }

Expand Down
4 changes: 2 additions & 2 deletions clang/test/CIR/global-var-simple.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>, ...)>>