[CIR][NFC] Eliminate ArgInfo structure #140612
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A previous refactoring had reduced the ArgInfo structure to contain a single member, the argument type. This change eliminates the ArgInfo structure entirely, instead just storing the argument type directly in places where ArgInfo had previously been used. This also updates the place where the arg types were previously being copied for a call to CIRGenFunctionInfo::Profile to instead use the stored argument types buffer directly and adds assertions where the calculated folding set ID is used to verify that any match was correct.
andykaylor
requested review from
Lancern,
bcardosolopes,
el-ev,
erichkeane,
lanza and
mmha
llvmbot
added
clang
Contributor
Author
|
This corresponds to the incubator PR at llvm/clangir#1629 |
Member
|
@llvm/pr-subscribers-clangir @llvm/pr-subscribers-clang Author: Andy Kaylor (andykaylor) ChangesA previous refactoring had reduced the ArgInfo structure to contain a single member, the argument type. This change eliminates the ArgInfo structure entirely, instead just storing the argument type directly in places where ArgInfo had previously been used. This also updates the place where the arg types were previously being copied for a call to CIRGenFunctionInfo::Profile to instead use the stored argument types buffer directly and adds assertions where the calculated folding set ID is used to verify that any match was correct. Full diff: https://github.com/llvm/llvm-project/pull/140612.diff 3 Files Affected:
diff --git a/clang/lib/CIR/CodeGen/CIRGenCall.cpp b/clang/lib/CIR/CodeGen/CIRGenCall.cpp
index 17bfa19f9fd63..f63b971ac26b4 100644
--- a/clang/lib/CIR/CodeGen/CIRGenCall.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenCall.cpp
@@ -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());
@@ -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;
@@ -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()),
@@ -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())
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunctionInfo.h b/clang/lib/CIR/CodeGen/CIRGenFunctionInfo.h
index b74460b09a44e..334b2864a2fb2 100644
--- a/clang/lib/CIR/CodeGen/CIRGenFunctionInfo.h
+++ b/clang/lib/CIR/CodeGen/CIRGenFunctionInfo.h
@@ -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 {
@@ -72,16 +68,15 @@ class RequiredArgs {
class CIRGenFunctionInfo final
: public llvm::FoldingSetNode,
- private llvm::TrailingObjects<CIRGenFunctionInfo,
- CIRGenFunctionInfoArgInfo> {
- using ArgInfo = CIRGenFunctionInfoArgInfo;
-
+ private llvm::TrailingObjects<CIRGenFunctionInfo, CanQualType> {
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) {}
@@ -96,8 +91,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)
@@ -112,49 +107,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();
}
};
diff --git a/clang/lib/CIR/CodeGen/CIRGenTypes.cpp b/clang/lib/CIR/CodeGen/CIRGenTypes.cpp
index dc8872122995c..45990f198d227 100644
--- a/clang/lib/CIR/CodeGen/CIRGenTypes.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenTypes.cpp
@@ -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");
return *fi;
+ }
assert(!cir::MissingFeatures::opCallCallConv());
|
erichkeane
approved these changes
May 19, 2025
andykaylor
commented
May 19, 2025
| assert( | ||
| fi->getReturnType() == returnType && | ||
| std::equal(fi->argTypesBegin(), fi->argTypesEnd(), argTypes.begin()) && | ||
| "Bad match based on CIRGenFunctionInfo folding set id"); |
Contributor
Author
There was a problem hiding this comment.
@bcardosolopes I think this assert accomplishes the testing you asked for in #140322 thoroughly without the need to introduce a unit test.
bcardosolopes
approved these changes
May 20, 2025
| assert( | ||
| fi->getReturnType() == returnType && | ||
| std::equal(fi->argTypesBegin(), fi->argTypesEnd(), argTypes.begin()) && | ||
| "Bad match based on CIRGenFunctionInfo folding set id"); |
Lancern
approved these changes
May 20, 2025
| CIRGenFunctionInfoArgInfo> { | ||
| using ArgInfo = CIRGenFunctionInfoArgInfo; | ||
|
|
||
| private llvm::TrailingObjects<CIRGenFunctionInfo, CanQualType> { |
Member
There was a problem hiding this comment.
It would be nice if we have some comment here to indicate that CanQualType refers to the argument type.
Contributor
Author
There was a problem hiding this comment.
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.
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/10/builds/5735 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A previous refactoring had reduced the ArgInfo structure to contain a single member, the argument type. This change eliminates the ArgInfo structure entirely, instead just storing the argument type directly in places where ArgInfo had previously been used.
This also updates the place where the arg types were previously being copied for a call to CIRGenFunctionInfo::Profile to instead use the stored argument types buffer directly and adds assertions where the calculated folding set ID is used to verify that any match was correct.