Skip to content

[CIR] Upstream emitAndUpdateRetAlloca #129933

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 1 commit into from
+51 −16
Closed

[CIR] Upstream emitAndUpdateRetAlloca #129933

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 22 additions & 0 deletions clang/lib/CIR/CodeGen/CIRGenFunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Collaborator

@erichkeane erichkeane Mar 5, 2025

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

@erichkeane erichkeane Mar 5, 2025

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

@andykaylor andykaylor Mar 5, 2025

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.

emitAndUpdateRetAlloca(fnRetQualTy, fnEndLoc,
getContext().getTypeAlignInChars(fnRetQualTy));
}

void CIRGenFunction::finishFunction(SourceLocation endLoc) {}
Expand All @@ -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();
Copy link
Collaborator

@erichkeane erichkeane Mar 5, 2025

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

if (!fnRetQualTy->isVoidType()) {
fnRetCIRTy = convertType(fnRetQualTy);
}

SourceLocation loc = funcDecl->getLocation();
Stmt *body = funcDecl->getBody();
SourceRange bodyRange =
Expand Down Expand Up @@ -312,4 +326,12 @@ LValue CIRGenFunction::emitLValue(const Expr *e) {
}
}

void CIRGenFunction::emitAndUpdateRetAlloca(QualType ty, mlir::Location loc,
CharUnits alignment) {
if (ty->isVoidType()) {
return;
}

Copy link
Contributor

@andykaylor andykaylor Mar 5, 2025

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.

emitAlloca("__retval", convertType(ty), loc, alignment);
}
} // namespace clang::CIRGen
5 changes: 5 additions & 0 deletions clang/lib/CIR/CodeGen/CIRGenFunction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ class CIRGenFunction : public CIRGenTypeCache {

public:
clang::QualType fnRetTy;
Copy link
Collaborator

@erichkeane erichkeane Mar 5, 2025

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

clang::QualType fnRetQualTy;
std::optional<mlir::Type> fnRetCIRTy;

/// This is the current function or global initializer that is generated code
/// for.
Expand Down Expand Up @@ -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);
Expand Down
40 changes: 24 additions & 16 deletions clang/test/CIR/func-simple.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Copy link
Contributor

@andykaylor andykaylor Mar 5, 2025

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}

// CHECK: %1 = cir.const #cir.int<42> : !cir.int<s, 32>
Copy link
Contributor

@andykaylor andykaylor Mar 5, 2025

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.return %1 : !cir.int<s, 32>
// CHECK: }

int scopes() {
Expand All @@ -25,47 +26,54 @@ 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
// CHECK: }

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