Skip to content

Conversation

@AmrDeveloper
Copy link
Member

This change adds support for the basic case of emitAndUpdateRetAlloca

@llvmbot llvmbot added clang Clang issues not falling into any other category ClangIR Anything related to the ClangIR project labels Mar 5, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 5, 2025

@llvm/pr-subscribers-clang

Author: Amr Hesham (AmrDeveloper)

Changes

This change adds support for the basic case of emitAndUpdateRetAlloca


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

3 Files Affected:

  • (modified) clang/lib/CIR/CodeGen/CIRGenFunction.cpp (+22)
  • (modified) clang/lib/CIR/CodeGen/CIRGenFunction.h (+5)
  • (modified) clang/test/CIR/func-simple.cpp (+24-16)
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.cpp b/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
index 7861a48c93244..69f29826d5ab0 100644
--- a/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
@@ -195,7 +195,16 @@ void CIRGenFunction::startFunction(GlobalDecl gd, QualType returnType,
     mlir::Location fnBodyBegin = getLoc(fd->getBody()->getBeginLoc());
     builder.CIRBaseBuilderTy::createStore(fnBodyBegin, paramVal, addrVal);
   }
+
   assert(builder.getInsertionBlock() && "Should be valid");
+
+  auto fnEndLoc = getLoc(fd->getBody()->getEndLoc());
+
+  // When the current function is not void, create an address to store the
+  // result value.
+  if (fnRetCIRTy.has_value())
+    emitAndUpdateRetAlloca(fnRetQualTy, fnEndLoc,
+                           getContext().getTypeAlignInChars(fnRetQualTy));
 }
 
 void CIRGenFunction::finishFunction(SourceLocation endLoc) {}
@@ -213,6 +222,11 @@ mlir::LogicalResult CIRGenFunction::emitFunctionBody(const clang::Stmt *body) {
 cir::FuncOp CIRGenFunction::generateCode(clang::GlobalDecl gd, cir::FuncOp fn,
                                          cir::FuncType funcType) {
   const auto funcDecl = cast<FunctionDecl>(gd.getDecl());
+  fnRetQualTy = funcDecl->getReturnType();
+  if (!fnRetQualTy->isVoidType()) {
+    fnRetCIRTy = convertType(fnRetQualTy);
+  }
+
   SourceLocation loc = funcDecl->getLocation();
   Stmt *body = funcDecl->getBody();
   SourceRange bodyRange =
@@ -312,4 +326,12 @@ LValue CIRGenFunction::emitLValue(const Expr *e) {
   }
 }
 
+void CIRGenFunction::emitAndUpdateRetAlloca(QualType ty, mlir::Location loc,
+                                            CharUnits alignment) {
+  if (ty->isVoidType()) {
+    return;
+  }
+
+  emitAlloca("__retval", convertType(ty), loc, alignment);
+}
 } // namespace clang::CIRGen
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.h b/clang/lib/CIR/CodeGen/CIRGenFunction.h
index 6b383378ae764..482f0e219a299 100644
--- a/clang/lib/CIR/CodeGen/CIRGenFunction.h
+++ b/clang/lib/CIR/CodeGen/CIRGenFunction.h
@@ -49,6 +49,8 @@ class CIRGenFunction : public CIRGenTypeCache {
 
 public:
   clang::QualType fnRetTy;
+  clang::QualType fnRetQualTy;
+  std::optional<mlir::Type> fnRetCIRTy;
 
   /// This is the current function or global initializer that is generated code
   /// for.
@@ -101,6 +103,9 @@ class CIRGenFunction : public CIRGenTypeCache {
                               clang::QualType ty, mlir::Location loc,
                               clang::CharUnits alignment, bool isParam = false);
 
+  void emitAndUpdateRetAlloca(clang::QualType ty, mlir::Location loc,
+                              clang::CharUnits alignment);
+
 public:
   mlir::Value emitAlloca(llvm::StringRef name, mlir::Type ty,
                          mlir::Location loc, clang::CharUnits alignment);
diff --git a/clang/test/CIR/func-simple.cpp b/clang/test/CIR/func-simple.cpp
index d37ccc7229f22..a8488d628d8f7 100644
--- a/clang/test/CIR/func-simple.cpp
+++ b/clang/test/CIR/func-simple.cpp
@@ -13,8 +13,9 @@ void voidret() { return; }
 
 int intfunc() { return 42; }
 // CHECK: cir.func @intfunc() -> !cir.int<s, 32> {
-// CHECK:   %0 = cir.const #cir.int<42> : !cir.int<s, 32>
-// CHECK:   cir.return %0 : !cir.int<s, 32>
+// CHECK:   %0 = cir.alloca !cir.int<s, 32>, !cir.ptr<!cir.int<s, 32>>, ["__retval"] {alignment = 4 : i64}
+// CHECK:   %1 = cir.const #cir.int<42> : !cir.int<s, 32>
+// CHECK:   cir.return %1 : !cir.int<s, 32>
 // CHECK: }
 
 int scopes() {
@@ -25,10 +26,11 @@ int scopes() {
   }
 }
 // CHECK: cir.func @scopes() -> !cir.int<s, 32> {
+// CHECK:   %0 = cir.alloca !cir.int<s, 32>, !cir.ptr<!cir.int<s, 32>>, ["__retval"] {alignment = 4 : i64}
 // CHECK:   cir.scope {
 // CHECK:     cir.scope {
-// CHECK:       %0 = cir.const #cir.int<99> : !cir.int<s, 32>
-// CHECK:       cir.return %0 : !cir.int<s, 32>
+// CHECK:       %1 = cir.const #cir.int<99> : !cir.int<s, 32>
+// CHECK:       cir.return %1 : !cir.int<s, 32>
 // CHECK:     }
 // CHECK:   }
 // CHECK:   cir.trap
@@ -36,36 +38,42 @@ int scopes() {
 
 long longfunc() { return 42l; }
 // CHECK: cir.func @longfunc() -> !cir.int<s, 64>
-// CHECK:   %0 = cir.const #cir.int<42> : !cir.int<s, 64>
-// CHECK:   cir.return %0 : !cir.int<s, 64>
+// CHECK:   %0 = cir.alloca !cir.int<s, 64>, !cir.ptr<!cir.int<s, 64>>, ["__retval"] {alignment = 8 : i64}
+// CHECK:   %1 = cir.const #cir.int<42> : !cir.int<s, 64>
+// CHECK:   cir.return %1 : !cir.int<s, 64>
 // CHECK: }
 
 unsigned unsignedfunc() { return 42u; }
 // CHECK: cir.func @unsignedfunc() -> !cir.int<u, 32>
-// CHECK:   %0 = cir.const #cir.int<42> : !cir.int<u, 32>
-// CHECK:   cir.return %0 : !cir.int<u, 32>
+// CHECK:   %0 = cir.alloca !cir.int<u, 32>, !cir.ptr<!cir.int<u, 32>>, ["__retval"] {alignment = 4 : i64}
+// CHECK:   %1 = cir.const #cir.int<42> : !cir.int<u, 32>
+// CHECK:   cir.return %1 : !cir.int<u, 32>
 // CHECK: }
 
 unsigned long long ullfunc() { return 42ull; }
 // CHECK: cir.func @ullfunc() -> !cir.int<u, 64>
-// CHECK:   %0 = cir.const #cir.int<42> : !cir.int<u, 64>
-// CHECK:   cir.return %0 : !cir.int<u, 64>
+// CHECK:   %0 = cir.alloca !cir.int<u, 64>, !cir.ptr<!cir.int<u, 64>>, ["__retval"] {alignment = 8 : i64}
+// CHECK:   %1 = cir.const #cir.int<42> : !cir.int<u, 64>
+// CHECK:   cir.return %1 : !cir.int<u, 64>
 // CHECK: }
 
 bool boolfunc() { return true; }
 // CHECK: cir.func @boolfunc() -> !cir.bool {
-// CHECK:   %0 = cir.const #true
-// CHECK:   cir.return %0 : !cir.bool
+// CHECK:   %0 = cir.alloca !cir.bool, !cir.ptr<!cir.bool>, ["__retval"] {alignment = 1 : i64}
+// CHECK:   %1 = cir.const #true
+// CHECK:   cir.return %1 : !cir.bool
 // CHECK: }
 
 float floatfunc() { return 42.42f; }
 // CHECK: cir.func @floatfunc() -> !cir.float {
-// CHECK:   %0 = cir.const #cir.fp<4.242
-// CHECK:   cir.return %0 : !cir.float
+// CHECK:   %0 = cir.alloca !cir.float, !cir.ptr<!cir.float>, ["__retval"] {alignment = 4 : i64}
+// CHECK:   %1 = cir.const #cir.fp<4.242
+// CHECK:   cir.return %1 : !cir.float
 // CHECK: }
 
 double doublefunc() { return 42.42; }
 // CHECK: cir.func @doublefunc() -> !cir.double {
-// CHECK:   %0 = cir.const #cir.fp<4.242
-// CHECK:   cir.return %0 : !cir.double
+// CHECK:   %0 = cir.alloca !cir.double, !cir.ptr<!cir.double>, ["__retval"] {alignment = 8 : i64}
+// CHECK:   %1 = cir.const #cir.fp<4.242
+// CHECK:   cir.return %1 : !cir.double
 // CHECK: }

@llvmbot
Copy link
Member

llvmbot commented Mar 5, 2025

@llvm/pr-subscribers-clangir

Author: Amr Hesham (AmrDeveloper)

Changes

This change adds support for the basic case of emitAndUpdateRetAlloca


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

3 Files Affected:

  • (modified) clang/lib/CIR/CodeGen/CIRGenFunction.cpp (+22)
  • (modified) clang/lib/CIR/CodeGen/CIRGenFunction.h (+5)
  • (modified) clang/test/CIR/func-simple.cpp (+24-16)
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.cpp b/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
index 7861a48c93244..69f29826d5ab0 100644
--- a/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
@@ -195,7 +195,16 @@ void CIRGenFunction::startFunction(GlobalDecl gd, QualType returnType,
     mlir::Location fnBodyBegin = getLoc(fd->getBody()->getBeginLoc());
     builder.CIRBaseBuilderTy::createStore(fnBodyBegin, paramVal, addrVal);
   }
+
   assert(builder.getInsertionBlock() && "Should be valid");
+
+  auto fnEndLoc = getLoc(fd->getBody()->getEndLoc());
+
+  // When the current function is not void, create an address to store the
+  // result value.
+  if (fnRetCIRTy.has_value())
+    emitAndUpdateRetAlloca(fnRetQualTy, fnEndLoc,
+                           getContext().getTypeAlignInChars(fnRetQualTy));
 }
 
 void CIRGenFunction::finishFunction(SourceLocation endLoc) {}
@@ -213,6 +222,11 @@ mlir::LogicalResult CIRGenFunction::emitFunctionBody(const clang::Stmt *body) {
 cir::FuncOp CIRGenFunction::generateCode(clang::GlobalDecl gd, cir::FuncOp fn,
                                          cir::FuncType funcType) {
   const auto funcDecl = cast<FunctionDecl>(gd.getDecl());
+  fnRetQualTy = funcDecl->getReturnType();
+  if (!fnRetQualTy->isVoidType()) {
+    fnRetCIRTy = convertType(fnRetQualTy);
+  }
+
   SourceLocation loc = funcDecl->getLocation();
   Stmt *body = funcDecl->getBody();
   SourceRange bodyRange =
@@ -312,4 +326,12 @@ LValue CIRGenFunction::emitLValue(const Expr *e) {
   }
 }
 
+void CIRGenFunction::emitAndUpdateRetAlloca(QualType ty, mlir::Location loc,
+                                            CharUnits alignment) {
+  if (ty->isVoidType()) {
+    return;
+  }
+
+  emitAlloca("__retval", convertType(ty), loc, alignment);
+}
 } // namespace clang::CIRGen
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.h b/clang/lib/CIR/CodeGen/CIRGenFunction.h
index 6b383378ae764..482f0e219a299 100644
--- a/clang/lib/CIR/CodeGen/CIRGenFunction.h
+++ b/clang/lib/CIR/CodeGen/CIRGenFunction.h
@@ -49,6 +49,8 @@ class CIRGenFunction : public CIRGenTypeCache {
 
 public:
   clang::QualType fnRetTy;
+  clang::QualType fnRetQualTy;
+  std::optional<mlir::Type> fnRetCIRTy;
 
   /// This is the current function or global initializer that is generated code
   /// for.
@@ -101,6 +103,9 @@ class CIRGenFunction : public CIRGenTypeCache {
                               clang::QualType ty, mlir::Location loc,
                               clang::CharUnits alignment, bool isParam = false);
 
+  void emitAndUpdateRetAlloca(clang::QualType ty, mlir::Location loc,
+                              clang::CharUnits alignment);
+
 public:
   mlir::Value emitAlloca(llvm::StringRef name, mlir::Type ty,
                          mlir::Location loc, clang::CharUnits alignment);
diff --git a/clang/test/CIR/func-simple.cpp b/clang/test/CIR/func-simple.cpp
index d37ccc7229f22..a8488d628d8f7 100644
--- a/clang/test/CIR/func-simple.cpp
+++ b/clang/test/CIR/func-simple.cpp
@@ -13,8 +13,9 @@ void voidret() { return; }
 
 int intfunc() { return 42; }
 // CHECK: cir.func @intfunc() -> !cir.int<s, 32> {
-// CHECK:   %0 = cir.const #cir.int<42> : !cir.int<s, 32>
-// CHECK:   cir.return %0 : !cir.int<s, 32>
+// CHECK:   %0 = cir.alloca !cir.int<s, 32>, !cir.ptr<!cir.int<s, 32>>, ["__retval"] {alignment = 4 : i64}
+// CHECK:   %1 = cir.const #cir.int<42> : !cir.int<s, 32>
+// CHECK:   cir.return %1 : !cir.int<s, 32>
 // CHECK: }
 
 int scopes() {
@@ -25,10 +26,11 @@ int scopes() {
   }
 }
 // CHECK: cir.func @scopes() -> !cir.int<s, 32> {
+// CHECK:   %0 = cir.alloca !cir.int<s, 32>, !cir.ptr<!cir.int<s, 32>>, ["__retval"] {alignment = 4 : i64}
 // CHECK:   cir.scope {
 // CHECK:     cir.scope {
-// CHECK:       %0 = cir.const #cir.int<99> : !cir.int<s, 32>
-// CHECK:       cir.return %0 : !cir.int<s, 32>
+// CHECK:       %1 = cir.const #cir.int<99> : !cir.int<s, 32>
+// CHECK:       cir.return %1 : !cir.int<s, 32>
 // CHECK:     }
 // CHECK:   }
 // CHECK:   cir.trap
@@ -36,36 +38,42 @@ int scopes() {
 
 long longfunc() { return 42l; }
 // CHECK: cir.func @longfunc() -> !cir.int<s, 64>
-// CHECK:   %0 = cir.const #cir.int<42> : !cir.int<s, 64>
-// CHECK:   cir.return %0 : !cir.int<s, 64>
+// CHECK:   %0 = cir.alloca !cir.int<s, 64>, !cir.ptr<!cir.int<s, 64>>, ["__retval"] {alignment = 8 : i64}
+// CHECK:   %1 = cir.const #cir.int<42> : !cir.int<s, 64>
+// CHECK:   cir.return %1 : !cir.int<s, 64>
 // CHECK: }
 
 unsigned unsignedfunc() { return 42u; }
 // CHECK: cir.func @unsignedfunc() -> !cir.int<u, 32>
-// CHECK:   %0 = cir.const #cir.int<42> : !cir.int<u, 32>
-// CHECK:   cir.return %0 : !cir.int<u, 32>
+// CHECK:   %0 = cir.alloca !cir.int<u, 32>, !cir.ptr<!cir.int<u, 32>>, ["__retval"] {alignment = 4 : i64}
+// CHECK:   %1 = cir.const #cir.int<42> : !cir.int<u, 32>
+// CHECK:   cir.return %1 : !cir.int<u, 32>
 // CHECK: }
 
 unsigned long long ullfunc() { return 42ull; }
 // CHECK: cir.func @ullfunc() -> !cir.int<u, 64>
-// CHECK:   %0 = cir.const #cir.int<42> : !cir.int<u, 64>
-// CHECK:   cir.return %0 : !cir.int<u, 64>
+// CHECK:   %0 = cir.alloca !cir.int<u, 64>, !cir.ptr<!cir.int<u, 64>>, ["__retval"] {alignment = 8 : i64}
+// CHECK:   %1 = cir.const #cir.int<42> : !cir.int<u, 64>
+// CHECK:   cir.return %1 : !cir.int<u, 64>
 // CHECK: }
 
 bool boolfunc() { return true; }
 // CHECK: cir.func @boolfunc() -> !cir.bool {
-// CHECK:   %0 = cir.const #true
-// CHECK:   cir.return %0 : !cir.bool
+// CHECK:   %0 = cir.alloca !cir.bool, !cir.ptr<!cir.bool>, ["__retval"] {alignment = 1 : i64}
+// CHECK:   %1 = cir.const #true
+// CHECK:   cir.return %1 : !cir.bool
 // CHECK: }
 
 float floatfunc() { return 42.42f; }
 // CHECK: cir.func @floatfunc() -> !cir.float {
-// CHECK:   %0 = cir.const #cir.fp<4.242
-// CHECK:   cir.return %0 : !cir.float
+// CHECK:   %0 = cir.alloca !cir.float, !cir.ptr<!cir.float>, ["__retval"] {alignment = 4 : i64}
+// CHECK:   %1 = cir.const #cir.fp<4.242
+// CHECK:   cir.return %1 : !cir.float
 // CHECK: }
 
 double doublefunc() { return 42.42; }
 // CHECK: cir.func @doublefunc() -> !cir.double {
-// CHECK:   %0 = cir.const #cir.fp<4.242
-// CHECK:   cir.return %0 : !cir.double
+// CHECK:   %0 = cir.alloca !cir.double, !cir.ptr<!cir.double>, ["__retval"] {alignment = 8 : i64}
+// CHECK:   %1 = cir.const #cir.fp<4.242
+// CHECK:   cir.return %1 : !cir.double
 // CHECK: }


assert(builder.getInsertionBlock() && "Should be valid");

auto fnEndLoc = getLoc(fd->getBody()->getEndLoc());
Copy link
Collaborator

Choose a reason for hiding this comment

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

not a place we can use auto I think. Also, could this just inlined in the call rather than doing the work to look it up even if we don't use it?


// When the current function is not void, create an address to store the
// result value.
if (fnRetCIRTy.has_value())
Copy link
Collaborator

Choose a reason for hiding this comment

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

What purpose does this fnRetCIRTy serve? startFunction already has the return type in returnType, right? Could/should we just convert it instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be mimicking what the LLVM IR codegen does with ReturnValue, but I agree that it's not necessary. All we really need to do here is check that the return type isn't void.

cir::FuncOp CIRGenFunction::generateCode(clang::GlobalDecl gd, cir::FuncOp fn,
cir::FuncType funcType) {
const auto funcDecl = cast<FunctionDecl>(gd.getDecl());
fnRetQualTy = funcDecl->getReturnType();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This whole thing doesn't seem useful as far as I can tell, we should just be able to use the return type in startFunction

CIRGenBuilderTy &builder;

public:
clang::QualType fnRetTy;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I question the need for fnRetTy too, particularly since fnRetQualTy and fnRetTy are not being set together...


// When the current function is not void, create an address to store the
// result value.
if (fnRetCIRTy.has_value())
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be mimicking what the LLVM IR codegen does with ReturnValue, but I agree that it's not necessary. All we really need to do here is check that the return type isn't void.

if (ty->isVoidType()) {
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

You should bring in the extra checks from the incubator for cases that are marked llvm_unreachable("NYI"); but use cgm.errorNYI() instead.

// CHECK: %0 = cir.const #cir.int<42> : !cir.int<s, 32>
// CHECK: cir.return %0 : !cir.int<s, 32>
// CHECK: %0 = cir.alloca !cir.int<s, 32>, !cir.ptr<!cir.int<s, 32>>, ["__retval"] {alignment = 4 : i64}
// CHECK: %1 = cir.const #cir.int<42> : !cir.int<s, 32>
Copy link
Contributor

Choose a reason for hiding this comment

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

The constant here should be stored to the return alloca slot and reloaded before being used in the return statement. That will require setting returnStatement here and using it in emitReturnStmt. Hopefully, that will cause updates to the test cases below.

// CHECK: cir.func @intfunc() -> !cir.int<s, 32> {
// CHECK: %0 = cir.const #cir.int<42> : !cir.int<s, 32>
// CHECK: cir.return %0 : !cir.int<s, 32>
// CHECK: %0 = cir.alloca !cir.int<s, 32>, !cir.ptr<!cir.int<s, 32>>, ["__retval"] {alignment = 4 : i64}
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize you were just following the style of the existing test, but in general it's preferable to use a substitution variable for IR identifiers that need to be matched or omit them from the check if they don't need to be matched.

Suggested change
// CHECK: %0 = cir.alloca !cir.int<s, 32>, !cir.ptr<!cir.int<s, 32>>, ["__retval"] {alignment = 4 : i64}
// CHECK: %[[RET_ALLOCA:.*]] = cir.alloca !cir.int<s, 32>, !cir.ptr<!cir.int<s, 32>>, ["__retval"] {alignment = 4 : i64}

@dkolsen-pgi
Copy link
Contributor

I have mixed feelings about this change. It mostly overlaps with what I am working on, which is class LexicalScope, which has a bunch of the code for handling the return value. I think I would prefer that this PR be dropped, as the changes will be part of the PR I am working on.

@AmrDeveloper
Copy link
Member Author

I have mixed feelings about this change. It mostly overlaps with what I am working on, which is class LexicalScope, which has a bunch of the code for handling the return value. I think I would prefer that this PR be dropped, as the changes will be part of the PR I am working on.

Yes, I think in this case it's better to drop this PR

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang Clang issues not falling into any other category ClangIR Anything related to the ClangIR project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants