Skip to content

[CIR][NFC] Eliminate ArgInfo structure #140612

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

Merged
merged 2 commits into from
May 20, 2025
Merged

[CIR][NFC] Eliminate ArgInfo structure #140612

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
2310c8a
[CIR][NFC] Eliminate ArgInfo structure
andykaylor May 19, 2025
3d99627
Add comment about TrailingObjects for CIRGenFunctionInfo
andykaylor May 20, 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
21 changes: 10 additions & 11 deletions clang/lib/CIR/CodeGen/CIRGenCall.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,9 @@ CIRGenFunctionInfo *
CIRGenFunctionInfo::create(CanQualType resultType,
llvm::ArrayRef<CanQualType> argTypes,
RequiredArgs required) {
// The first slot allocated for ArgInfo is for the return value.
void *buffer = operator new(totalSizeToAlloc<ArgInfo>(argTypes.size() + 1));
// The first slot allocated for arg type slot is for the return value.
void *buffer = operator new(
totalSizeToAlloc<CanQualType>(argTypes.size() + 1));

assert(!cir::MissingFeatures::opCallCIRGenFuncInfoParamInfo());

Expand All @@ -33,10 +34,8 @@ CIRGenFunctionInfo::create(CanQualType resultType,
fi->required = required;
fi->numArgs = argTypes.size();

ArgInfo *argsBuffer = fi->getArgsBuffer();
(argsBuffer++)->type = resultType;
for (CanQualType ty : argTypes)
(argsBuffer++)->type = ty;
fi->getArgTypes()[0] = resultType;
std::copy(argTypes.begin(), argTypes.end(), fi->argTypesBegin());
assert(!cir::MissingFeatures::opCallCIRGenFuncInfoExtParamInfo());

return fi;
Expand All @@ -47,8 +46,8 @@ cir::FuncType CIRGenTypes::getFunctionType(const CIRGenFunctionInfo &fi) {
SmallVector<mlir::Type, 8> argTypes;
argTypes.reserve(fi.getNumRequiredArgs());

for (const CIRGenFunctionInfoArgInfo &argInfo : fi.requiredArguments())
argTypes.push_back(convertType(argInfo.type));
for (const CanQualType &argType : fi.requiredArguments())
argTypes.push_back(convertType(argType));

return cir::FuncType::get(argTypes,
(resultType ? resultType : builder.getVoidTy()),
Expand Down Expand Up @@ -189,13 +188,13 @@ RValue CIRGenFunction::emitCall(const CIRGenFunctionInfo &funcInfo,
assert(!cir::MissingFeatures::emitLifetimeMarkers());

// Translate all of the arguments as necessary to match the CIR lowering.
for (auto [argNo, arg, argInfo] :
llvm::enumerate(args, funcInfo.argInfos())) {
for (auto [argNo, arg, canQualArgType] :
llvm::enumerate(args, funcInfo.argTypes())) {

// Insert a padding argument to ensure proper alignment.
assert(!cir::MissingFeatures::opCallPaddingArgs());

mlir::Type argType = convertType(argInfo.type);
mlir::Type argType = convertType(canQualArgType);
if (!mlir::isa<cir::RecordType>(argType)) {
mlir::Value v;
if (arg.isAggregate())
Expand Down
67 changes: 28 additions & 39 deletions clang/lib/CIR/CodeGen/CIRGenFunctionInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,6 @@

namespace clang::CIRGen {

struct CIRGenFunctionInfoArgInfo {
CanQualType type;
};

/// A class for recording the number of arguments that a function signature
/// requires.
class RequiredArgs {
Expand Down Expand Up @@ -70,18 +66,19 @@ class RequiredArgs {
}
};

// The TrailingObjects for this class contain the function return type in the
// first CanQualType slot, followed by the argument types.
class CIRGenFunctionInfo final
: public llvm::FoldingSetNode,
private llvm::TrailingObjects<CIRGenFunctionInfo,
CIRGenFunctionInfoArgInfo> {
using ArgInfo = CIRGenFunctionInfoArgInfo;

private llvm::TrailingObjects<CIRGenFunctionInfo, CanQualType> {
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if we have some comment here to indicate that CanQualType refers to the argument type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strictly speaking, that part of the buffer is used for both the return type and the argument types, but I agree that a comment would be useful.

RequiredArgs required;

unsigned numArgs;

ArgInfo *getArgsBuffer() { return getTrailingObjects<ArgInfo>(); }
const ArgInfo *getArgsBuffer() const { return getTrailingObjects<ArgInfo>(); }
CanQualType *getArgTypes() { return getTrailingObjects<CanQualType>(); }
const CanQualType *getArgTypes() const {
return getTrailingObjects<CanQualType>();
}

CIRGenFunctionInfo() : required(RequiredArgs::All) {}

Expand All @@ -96,8 +93,8 @@ class CIRGenFunctionInfo final
// these have to be public.
friend class TrailingObjects;

using const_arg_iterator = const ArgInfo *;
using arg_iterator = ArgInfo *;
using const_arg_iterator = const CanQualType *;
using arg_iterator = CanQualType *;

// This function has to be CamelCase because llvm::FoldingSet requires so.
// NOLINTNEXTLINE(readability-identifier-naming)
Expand All @@ -112,49 +109,41 @@ class CIRGenFunctionInfo final

// NOLINTNEXTLINE(readability-identifier-naming)
void Profile(llvm::FoldingSetNodeID &id) {
// It's unfortunate that we are looping over the arguments twice (here and
// in the static Profile function we call from here), but if the Profile
// functions get out of sync, we can end up with incorrect function
// signatures, and we don't have the argument types in the format that the
// static Profile function requires.
llvm::SmallVector<CanQualType, 16> argTypes;
for (const ArgInfo &argInfo : arguments())
argTypes.push_back(argInfo.type);

Profile(id, required, getReturnType(), argTypes);
// If the Profile functions get out of sync, we can end up with incorrect
// function signatures, so we call the static Profile function here rather
// than duplicating the logic.
Profile(id, required, getReturnType(), arguments());
}

llvm::ArrayRef<ArgInfo> arguments() const {
return llvm::ArrayRef<ArgInfo>(argInfoBegin(), numArgs);
llvm::ArrayRef<CanQualType> arguments() const {
return llvm::ArrayRef<CanQualType>(argTypesBegin(), numArgs);
}

llvm::ArrayRef<ArgInfo> requiredArguments() const {
return llvm::ArrayRef<ArgInfo>(argInfoBegin(), getNumRequiredArgs());
llvm::ArrayRef<CanQualType> requiredArguments() const {
return llvm::ArrayRef<CanQualType>(argTypesBegin(), getNumRequiredArgs());
}

CanQualType getReturnType() const { return getArgsBuffer()[0].type; }
CanQualType getReturnType() const { return getArgTypes()[0]; }

const_arg_iterator argInfoBegin() const { return getArgsBuffer() + 1; }
const_arg_iterator argInfoEnd() const {
return getArgsBuffer() + 1 + numArgs;
}
arg_iterator argInfoBegin() { return getArgsBuffer() + 1; }
arg_iterator argInfoEnd() { return getArgsBuffer() + 1 + numArgs; }
const_arg_iterator argTypesBegin() const { return getArgTypes() + 1; }
const_arg_iterator argTypesEnd() const { return getArgTypes() + 1 + numArgs; }
arg_iterator argTypesBegin() { return getArgTypes() + 1; }
arg_iterator argTypesEnd() { return getArgTypes() + 1 + numArgs; }

unsigned argInfoSize() const { return numArgs; }
unsigned argTypeSize() const { return numArgs; }

llvm::MutableArrayRef<ArgInfo> argInfos() {
return llvm::MutableArrayRef<ArgInfo>(argInfoBegin(), numArgs);
llvm::MutableArrayRef<CanQualType> argTypes() {
return llvm::MutableArrayRef<CanQualType>(argTypesBegin(), numArgs);
}
llvm::ArrayRef<ArgInfo> argInfos() const {
return llvm::ArrayRef<ArgInfo>(argInfoBegin(), numArgs);
llvm::ArrayRef<CanQualType> argTypes() const {
return llvm::ArrayRef<CanQualType>(argTypesBegin(), numArgs);
}

bool isVariadic() const { return required.allowsOptionalArgs(); }
RequiredArgs getRequiredArgs() const { return required; }
unsigned getNumRequiredArgs() const {
return isVariadic() ? getRequiredArgs().getNumRequiredArgs()
: argInfoSize();
: argTypeSize();
}
};

Expand Down
9 changes: 8 additions & 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 @@ -542,8 +542,15 @@ CIRGenTypes::arrangeCIRFunctionInfo(CanQualType returnType,

void *insertPos = nullptr;
CIRGenFunctionInfo *fi = functionInfos.FindNodeOrInsertPos(id, insertPos);
if (fi)
if (fi) {
// We found a matching function info based on id. These asserts verify that
// it really is a match.
assert(
fi->getReturnType() == returnType &&
std::equal(fi->argTypesBegin(), fi->argTypesEnd(), argTypes.begin()) &&
"Bad match based on CIRGenFunctionInfo folding set id");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bcardosolopes I think this assert accomplishes the testing you asked for in #140322 thoroughly without the need to introduce a unit test.

Copy link
Member

Choose a reason for hiding this comment

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

thanks Andy!

return *fi;
}

assert(!cir::MissingFeatures::opCallCallConv());

Expand Down