[Clang][CodeGen][CIR][NFC] Refactor CGCXXABI classes for code sharing #165078
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
This refactors the CGCXXABI and ItaniumCXXABI classes in LLVM IR codegen and their CIR codegen counterparts so that common code can be shared between the two. This is a small first step towards better code sharing between LLVM IR codegen and CIR codegen. We should be able to expand this significantly as CIR codegen matures. Note that some other code can very nearly, but not quite be shared at this point. For example `AddedStructorArgs` and its uses are nearly identical between LLVM IR and CIR codegen, but the struct contains an `llvm::Value*` member in LLVM IR codegen where it conatins an `mlir::Value` member in CIR codegen. We will need additional refactoring to handle cases like that.
andykaylor
requested review from
AaronBallman,
asl,
bcardosolopes,
efriedma-quic,
erichkeane,
lanza,
ojhunt,
rjmccall and
xlauko
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clangir Author: Andy Kaylor (andykaylor) ChangesThis refactors the CGCXXABI and ItaniumCXXABI classes in LLVM IR codegen and their CIR codegen counterparts so that common code can be shared between the two. This is a small first step towards better code sharing between LLVM IR codegen and CIR codegen. We should be able to expand this significantly as CIR codegen matures. Note that some other code can very nearly, but not quite be shared at this point. For example Patch is 35.03 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/165078.diff 11 Files Affected:
diff --git a/clang/include/clang/CodeGenShared/CXXABIShared.h b/clang/include/clang/CodeGenShared/CXXABIShared.h
new file mode 100644
index 0000000000000..957aae749c53b
--- /dev/null
+++ b/clang/include/clang/CodeGenShared/CXXABIShared.h
@@ -0,0 +1,99 @@
+//===----- CXXABIShared.h - Shared C++ ABI Base Class -----------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file provides a base class for C++ ABI functionality that can be shared
+// between LLVM IR codegen and CIR codegen.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_CODEGENSHARED_CXXABISHARED_H
+#define LLVM_CLANG_CODEGENSHARED_CXXABISHARED_H
+
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/GlobalDecl.h"
+#include "clang/AST/Mangle.h"
+#include "clang/Basic/ABI.h"
+
+namespace clang {
+class ASTContext;
+
+/// Implements C++ ABI functionality that can be shared between LLVM IR codegen
+/// and CIR codegen.
+class CXXABIShared {
+protected:
+ ASTContext &Context;
+ std::unique_ptr<MangleContext> MangleCtx;
+
+ CXXABIShared(ASTContext &Context)
+ : Context(Context), MangleCtx(Context.createMangleContext()) {}
+
+public:
+ virtual ~CXXABIShared() = default;
+
+ /// Similar to AddedStructorArgs, but only notes the number of additional
+ /// arguments.
+ struct AddedStructorArgCounts {
+ unsigned Prefix = 0;
+ unsigned Suffix = 0;
+ AddedStructorArgCounts() = default;
+ AddedStructorArgCounts(unsigned P, unsigned S) : Prefix(P), Suffix(S) {}
+ static AddedStructorArgCounts prefix(unsigned N) { return {N, 0}; }
+ static AddedStructorArgCounts suffix(unsigned N) { return {0, N}; }
+ };
+
+ /// Get the AST context.
+ ASTContext &getContext() const { return Context; }
+
+ /// Gets the mangle context.
+ MangleContext &getMangleContext() { return *MangleCtx; }
+
+ /// Determine whether there's something special about the rules of
+ /// the ABI tell us that 'this' is a complete object within the
+ /// given function. Obvious common logic like being defined on a
+ /// final class will have been taken care of by the caller.
+ virtual bool isThisCompleteObject(GlobalDecl GD) const = 0;
+
+ /// Returns true if the most-derived return value should be returned.
+ virtual bool hasMostDerivedReturn(GlobalDecl GD) const { return false; }
+
+ /// Return whether the given global decl needs a VTT parameter.
+ virtual bool NeedsVTTParameter(GlobalDecl GD) const { return false; }
+
+ /// Returns true if the given constructor or destructor is one of the
+ /// kinds that the ABI says returns 'this' (only applies when called
+ /// non-virtually for destructors).
+ ///
+ /// There currently is no way to indicate if a destructor returns 'this'
+ /// when called virtually, and code generation does not support the case.
+ /// Returns true if the given constructor or destructor is one of the
+ /// kinds that the ABI says returns 'this' (only applies when called
+ /// non-virtually for destructors).
+ ///
+ /// There currently is no way to indicate if a destructor returns 'this'
+ /// when called virtually, and code generation does not support the case.
+ virtual bool HasThisReturn(GlobalDecl GD) const {
+ if (isa<CXXConstructorDecl>(GD.getDecl()) ||
+ (isa<CXXDestructorDecl>(GD.getDecl()) &&
+ GD.getDtorType() != Dtor_Deleting))
+ return constructorsAndDestructorsReturnThis();
+ return false;
+ }
+
+ /// Returns true if the given destructor type should be emitted as a linkonce
+ /// delegating thunk, regardless of whether the dtor is defined in this TU or
+ /// not.
+ virtual bool useThunkForDtorVariant(const CXXDestructorDecl *Dtor,
+ CXXDtorType DT) const = 0;
+
+protected:
+ virtual bool constructorsAndDestructorsReturnThis() const = 0;
+};
+
+} // namespace clang
+
+#endif // LLVM_CLANG_CODEGENSHARED_CXXABISHARED_H
diff --git a/clang/include/clang/CodeGenShared/ItaniumCXXABIShared.h b/clang/include/clang/CodeGenShared/ItaniumCXXABIShared.h
new file mode 100644
index 0000000000000..e1f1d1242aea3
--- /dev/null
+++ b/clang/include/clang/CodeGenShared/ItaniumCXXABIShared.h
@@ -0,0 +1,118 @@
+//===--- ItaniumCXXABIShared.h - Itanium C++ ABI Shared Base ----*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file provides a base class for Itanium C++ ABI implementations that can
+// be shared between LLVM IR codegen and CIR codegen.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_CODEGENSHARED_ITANIUMCXXABISHARED_H
+#define LLVM_CLANG_CODEGENSHARED_ITANIUMCXXABISHARED_H
+
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/GlobalDecl.h"
+#include "clang/Basic/ABI.h"
+#include <utility>
+
+namespace clang {
+
+template <typename BaseT> class ItaniumCXXABIShared : public BaseT {
+protected:
+ /// Constructor forwarding to the base ABI class.
+ template <typename... Args>
+ ItaniumCXXABIShared(Args &&...args) : BaseT(std::forward<Args>(args)...) {}
+
+public:
+ /// Determine whether there's something special about the rules of
+ /// the ABI tell us that 'this' is a complete object within the
+ /// given function. Obvious common logic like being defined on a
+ /// final class will have been taken care of by the caller.
+ bool isThisCompleteObject(GlobalDecl GD) const override {
+ // The Itanium ABI has separate complete-object vs. base-object
+ // variants of both constructors and destructors.
+ if (isa<CXXDestructorDecl>(GD.getDecl())) {
+ switch (GD.getDtorType()) {
+ case Dtor_Complete:
+ case Dtor_Deleting:
+ return true;
+
+ case Dtor_Base:
+ return false;
+
+ case Dtor_Comdat:
+ llvm_unreachable("emitting dtor comdat as function?");
+ case Dtor_Unified:
+ llvm_unreachable("emitting unified dtor as function?");
+ }
+ llvm_unreachable("bad dtor kind");
+ }
+ if (isa<CXXConstructorDecl>(GD.getDecl())) {
+ switch (GD.getCtorType()) {
+ case Ctor_Complete:
+ return true;
+
+ case Ctor_Base:
+ return false;
+
+ case Ctor_CopyingClosure:
+ case Ctor_DefaultClosure:
+ llvm_unreachable("closure ctors in Itanium ABI?");
+
+ case Ctor_Comdat:
+ llvm_unreachable("emitting ctor comdat as function?");
+
+ case Ctor_Unified:
+ llvm_unreachable("emitting unified ctor as function?");
+ }
+ llvm_unreachable("bad ctor kind");
+ }
+
+ // No other kinds.
+ return false;
+ }
+
+ bool NeedsVTTParameter(GlobalDecl GD) const override {
+ const CXXMethodDecl *MD = cast<CXXMethodDecl>(GD.getDecl());
+
+ // We don't have any virtual bases, just return early.
+ if (!MD->getParent()->getNumVBases())
+ return false;
+
+ // Check if we have a base constructor.
+ if (isa<CXXConstructorDecl>(MD) && GD.getCtorType() == Ctor_Base)
+ return true;
+
+ // Check if we have a base destructor.
+ if (isa<CXXDestructorDecl>(MD) && GD.getDtorType() == Dtor_Base)
+ return true;
+
+ return false;
+ }
+
+ /// Returns true if the given destructor type should be emitted as a linkonce
+ /// delegating thunk, regardless of whether the dtor is defined in this TU or
+ /// not.
+ ///
+ /// Itanium does not emit any destructor variant as an inline thunk.
+ /// Delegating may occur as an optimization, but all variants are either
+ /// emitted with external linkage or as linkonce if they are inline and used.
+ bool useThunkForDtorVariant(const CXXDestructorDecl *Dtor,
+ CXXDtorType DT) const override {
+ return false;
+ }
+
+ /// Returns true if this ABI initializes vptrs in constructors/destructors.
+ /// For Itanium, this is always true.
+ bool doStructorsInitializeVPtrs(const CXXRecordDecl *VTableClass) override {
+ return true;
+ }
+};
+
+} // namespace clang
+
+#endif // LLVM_CLANG_CODEGENSHARED_ITANIUMCXXABISHARED_H
diff --git a/clang/lib/CIR/CodeGen/CIRGenCXX.cpp b/clang/lib/CIR/CodeGen/CIRGenCXX.cpp
index a3e20817d2ca4..2fd6980499edd 100644
--- a/clang/lib/CIR/CodeGen/CIRGenCXX.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenCXX.cpp
@@ -116,7 +116,7 @@ static void emitDeclDestroy(CIRGenFunction &cgf, const VarDecl *vd,
// mismatch.
const CXXRecordDecl *record = type->getAsCXXRecordDecl();
bool canRegisterDestructor =
- record && (!cgm.getCXXABI().hasThisReturn(
+ record && (!cgm.getCXXABI().HasThisReturn(
GlobalDecl(record->getDestructor(), Dtor_Complete)) ||
cgm.getCXXABI().canCallMismatchedFunctionType());
diff --git a/clang/lib/CIR/CodeGen/CIRGenCXXABI.h b/clang/lib/CIR/CodeGen/CIRGenCXXABI.h
index 13dc9f305945a..f292ced83259f 100644
--- a/clang/lib/CIR/CodeGen/CIRGenCXXABI.h
+++ b/clang/lib/CIR/CodeGen/CIRGenCXXABI.h
@@ -19,23 +19,25 @@
#include "CIRGenFunction.h"
#include "CIRGenModule.h"
-#include "clang/AST/Mangle.h"
+#include "clang/CodeGenShared/CXXABIShared.h"
namespace clang::CIRGen {
/// Implements C++ ABI-specific code generation functions.
-class CIRGenCXXABI {
+class CIRGenCXXABI : public CXXABIShared {
protected:
CIRGenModule &cgm;
- std::unique_ptr<clang::MangleContext> mangleContext;
+
+ CIRGenCXXABI(CIRGenModule &cgm)
+ : CXXABIShared(cgm.getASTContext()), cgm(cgm) {}
virtual bool requiresArrayCookie(const CXXNewExpr *e);
+ bool constructorsAndDestructorsReturnThis() const override {
+ return cgm.getCodeGenOpts().CtorDtorReturnThis;
+ }
+
public:
- // TODO(cir): make this protected when target-specific CIRGenCXXABIs are
- // implemented.
- CIRGenCXXABI(CIRGenModule &cgm)
- : cgm(cgm), mangleContext(cgm.getASTContext().createMangleContext()) {}
virtual ~CIRGenCXXABI();
void setCXXABIThisValue(CIRGenFunction &cgf, mlir::Value thisPtr);
@@ -62,17 +64,6 @@ class CIRGenCXXABI {
bool isRefCast, Address src) = 0;
public:
- /// Similar to AddedStructorArgs, but only notes the number of additional
- /// arguments.
- struct AddedStructorArgCounts {
- unsigned prefix = 0;
- unsigned suffix = 0;
- AddedStructorArgCounts() = default;
- AddedStructorArgCounts(unsigned p, unsigned s) : prefix(p), suffix(s) {}
- static AddedStructorArgCounts withPrefix(unsigned n) { return {n, 0}; }
- static AddedStructorArgCounts withSuffix(unsigned n) { return {0, n}; }
- };
-
/// Additional implicit arguments to add to the beginning (Prefix) and end
/// (Suffix) of a constructor / destructor arg list.
///
@@ -138,10 +129,6 @@ class CIRGenCXXABI {
return md->getParent();
}
- /// Return whether the given global decl needs a VTT (virtual table table)
- /// parameter.
- virtual bool needsVTTParameter(clang::GlobalDecl gd) { return false; }
-
/// Perform ABI-specific "this" argument adjustment required prior to
/// a call of a virtual function.
/// The "VirtualCall" argument is true iff the call itself is virtual.
@@ -214,12 +201,6 @@ class CIRGenCXXABI {
/// this emits virtual table tables.
virtual void emitVirtualInheritanceTables(const CXXRecordDecl *rd) = 0;
- /// Returns true if the given destructor type should be emitted as a linkonce
- /// delegating thunk, regardless of whether the dtor is defined in this TU or
- /// not.
- virtual bool useThunkForDtorVariant(const CXXDestructorDecl *dtor,
- CXXDtorType dt) const = 0;
-
virtual cir::GlobalLinkageKind
getCXXDestructorLinkage(GVALinkage linkage, const CXXDestructorDecl *dtor,
CXXDtorType dt) const;
@@ -262,18 +243,6 @@ class CIRGenCXXABI {
virtual bool
doStructorsInitializeVPtrs(const clang::CXXRecordDecl *vtableClass) = 0;
- /// Returns true if the given constructor or destructor is one of the kinds
- /// that the ABI says returns 'this' (only applies when called non-virtually
- /// for destructors).
- ///
- /// There currently is no way to indicate if a destructor returns 'this' when
- /// called virtually, and CIR generation does not support this case.
- virtual bool hasThisReturn(clang::GlobalDecl gd) const { return false; }
-
- virtual bool hasMostDerivedReturn(clang::GlobalDecl gd) const {
- return false;
- }
-
/// Returns true if the target allows calling a function through a pointer
/// with a different signature than the actual function (or equivalently,
/// bitcasting a function or function pointer to a different function type).
@@ -284,9 +253,6 @@ class CIRGenCXXABI {
/// for all calls.
virtual bool canCallMismatchedFunctionType() const { return true; }
- /// Gets the mangle context.
- clang::MangleContext &getMangleContext() { return *mangleContext; }
-
clang::ImplicitParamDecl *&getStructorImplicitParamDecl(CIRGenFunction &cgf) {
return cgf.cxxStructorImplicitParamDecl;
}
diff --git a/clang/lib/CIR/CodeGen/CIRGenCall.cpp b/clang/lib/CIR/CodeGen/CIRGenCall.cpp
index 88aef89ddd2b9..f4fb20bc3368c 100644
--- a/clang/lib/CIR/CodeGen/CIRGenCall.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenCall.cpp
@@ -206,12 +206,12 @@ CIRGenTypes::arrangeCXXStructorDeclaration(GlobalDecl gd) {
(passParams && md->isVariadic() ? RequiredArgs(argTypes.size())
: RequiredArgs::All);
- CanQualType resultType = theCXXABI.hasThisReturn(gd) ? argTypes.front()
+ CanQualType resultType = theCXXABI.HasThisReturn(gd) ? argTypes.front()
: theCXXABI.hasMostDerivedReturn(gd)
? astContext.VoidPtrTy
: astContext.VoidTy;
- assert(!theCXXABI.hasThisReturn(gd) &&
+ assert(!theCXXABI.HasThisReturn(gd) &&
"Please send PR with a test and remove this");
assert(!cir::MissingFeatures::opCallCIRGenFuncInfoExtParamInfo());
@@ -350,7 +350,7 @@ const CIRGenFunctionInfo &CIRGenTypes::arrangeCXXConstructorCall(
: RequiredArgs::All;
GlobalDecl gd(d, ctorKind);
- if (theCXXABI.hasThisReturn(gd))
+ if (theCXXABI.HasThisReturn(gd))
cgm.errorNYI(d->getSourceRange(),
"arrangeCXXConstructorCall: hasThisReturn");
if (theCXXABI.hasMostDerivedReturn(gd))
diff --git a/clang/lib/CIR/CodeGen/CIRGenClass.cpp b/clang/lib/CIR/CodeGen/CIRGenClass.cpp
index 5046e0945002f..067e6d29ef526 100644
--- a/clang/lib/CIR/CodeGen/CIRGenClass.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenClass.cpp
@@ -759,7 +759,7 @@ void CIRGenFunction::emitDelegateCXXConstructorCall(
// FIXME: The location of the VTT parameter in the parameter list is specific
// to the Itanium ABI and shouldn't be hardcoded here.
- if (cgm.getCXXABI().needsVTTParameter(curGD)) {
+ if (cgm.getCXXABI().NeedsVTTParameter(curGD)) {
cgm.errorNYI(loc, "emitDelegateCXXConstructorCall: VTT parameter");
return;
}
@@ -1068,7 +1068,7 @@ void CIRGenFunction::emitCXXDestructorCall(const CXXDestructorDecl *dd,
mlir::Value CIRGenFunction::getVTTParameter(GlobalDecl gd, bool forVirtualBase,
bool delegating) {
- if (!cgm.getCXXABI().needsVTTParameter(gd))
+ if (!cgm.getCXXABI().NeedsVTTParameter(gd))
return nullptr;
const CXXRecordDecl *rd = cast<CXXMethodDecl>(curCodeDecl)->getParent();
@@ -1082,7 +1082,7 @@ mlir::Value CIRGenFunction::getVTTParameter(GlobalDecl gd, bool forVirtualBase,
} else if (rd == base) {
// If the record matches the base, this is the complete ctor/dtor
// variant calling the base variant in a class with virtual bases.
- assert(!cgm.getCXXABI().needsVTTParameter(curGD) &&
+ assert(!cgm.getCXXABI().NeedsVTTParameter(curGD) &&
"doing no-op VTT offset in base dtor/ctor?");
assert(!forVirtualBase && "Can't have same class as virtual base!");
subVTTIndex = 0;
@@ -1097,7 +1097,7 @@ mlir::Value CIRGenFunction::getVTTParameter(GlobalDecl gd, bool forVirtualBase,
}
mlir::Location loc = cgm.getLoc(rd->getBeginLoc());
- if (cgm.getCXXABI().needsVTTParameter(curGD)) {
+ if (cgm.getCXXABI().NeedsVTTParameter(curGD)) {
// A VTT parameter was passed to the constructor, use it.
mlir::Value vtt = loadCXXVTT();
return builder.createVTTAddrPoint(loc, vtt.getType(), vtt, subVTTIndex);
@@ -1266,7 +1266,7 @@ void CIRGenFunction::emitCXXConstructorCall(
// Emit the call.
auto calleePtr = cgm.getAddrOfCXXStructor(GlobalDecl(d, type));
const CIRGenFunctionInfo &info = cgm.getTypes().arrangeCXXConstructorCall(
- args, d, type, extraArgs.prefix, extraArgs.suffix, passPrototypeArgs);
+ args, d, type, extraArgs.Prefix, extraArgs.Suffix, passPrototypeArgs);
CIRGenCallee callee = CIRGenCallee::forDirect(calleePtr, GlobalDecl(d, type));
cir::CIRCallOpInterface c;
emitCall(info, callee, ReturnValueSlot(), args, &c, getLoc(loc));
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.cpp b/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
index 58feb36f78f23..d1affce25e429 100644
--- a/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
@@ -833,7 +833,7 @@ clang::QualType CIRGenFunction::buildFunctionArgList(clang::GlobalDecl gd,
const auto *md = dyn_cast<CXXMethodDecl>(fd);
if (md && md->isInstance()) {
- if (cgm.getCXXABI().hasThisReturn(gd))
+ if (cgm.getCXXABI().HasThisReturn(gd))
cgm.errorNYI(fd->getSourceRange(), "this return");
else if (cgm.getCXXABI().hasMostDerivedReturn(gd))
cgm.errorNYI(fd->getSourceRange(), "most derived return");
diff --git a/clang/lib/CIR/CodeGen/CIRGenItaniumCXXABI.cpp b/clang/lib/CIR/CodeGen/CIRGenItaniumCXXABI.cpp
index 88fedf1acc6a1..7c0732e85b0ac 100644
--- a/clang/lib/CIR/CodeGen/CIRGenItaniumCXXABI.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenItaniumCXXABI.cpp
@@ -24,6 +24,7 @@
#include "clang/AST/GlobalDecl.h"
#include "clang/AST/VTableBuilder.h"
#include "clang/CIR/MissingFeatures.h"
+#include "clang/CodeGenShared/ItaniumCXXABIShared.h"
#include "llvm/Support/ErrorHandling.h"
using namespace clang;
@@ -31,13 +32,14 @@ using namespace clang::CIRGen;
namespace {
-class CIRGenItaniumCXXABI : public CIRGenCXXABI {
+class CIRGenItaniumCXXABI : public ItaniumCXXABIShared<CIRGenCXXABI> {
protected:
/// All the vtables which have been defined.
llvm::DenseMap<const CXXRecordDecl *, cir::GlobalOp> vtables;
public:
- CIRGenItaniumCXXABI(CIRGenModule &cgm) : CIRGenCXXABI(cgm) {
+ CIRGenItaniumCXXABI(CIRGenModule &cgm)
+ : ItaniumCXXABIShared<CIRGenCXXABI>(cgm) {
assert(!cir::MissingFeatures::cxxabiUseARMMethodPtrABI());
assert(!cir::MissingFeatures::cxxabiUseARMGuardVarABI());
}
@@ -48,8 +50,6 @@ class CIRGenItaniumCXXABI : public CIRGenCXXABI {
bool forVirtualBase,
bool delegating) override;
- bool needsVTTParameter(clang::GlobalDecl gd) override;
-
AddedStructorArgCounts
buildStructorSignature(GlobalDecl gd,
llvm::SmallVectorImpl<CanQualType> &argTys) override;
@@ -81,13 +81,6 @@ class CIRGenItaniumCXXABI : public CIRGenCXXABI {
void emitRethrow(CIRGenFunction &cgf, bool isNoReturn) override;
void emitThrow(CIRGenFunction &cgf, const CXXThrowExpr *e) override;
- bool useThunkForDtorVariant(const CXXDestructorDecl *dtor,
- CXXDtorType dt) const override {
- // Itanium does not emit any destructor variant as an inline thunk.
- // Delegating may occur as an optimization, but all variants are either
- // emitted with external linkage or as linkonce if they are inline and used.
- return false;
- }
bool isVirtualOffsetNeededForVTableField(CIRGenFunction &cgf,
CIRGenFunction::VPtr vptr) override;
@@ -119,10 +112,6 @@ class CIRGenItaniumCXXABI : public CIRGenCXXABI {
mlir::Attribute getAddrOfRTTIDesc...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| virtual bool hasMostDerivedReturn(GlobalDecl GD) const { return false; } | ||
|
|
||
| /// Return whether the given global decl needs a VTT parameter. | ||
| virtual bool NeedsVTTParameter(GlobalDecl GD) const { return false; } |
There was a problem hiding this comment.
I noticed there's a mismatch of style among the methods, they should be consistent. Not sure which one we want to stick with given clang vs MLIR differences, but I'd vote for camelBack for this!
There was a problem hiding this comment.
I believe we should match the style of the rest of clang - I'm somewhat concerned about the current divergence from the clang naming style (although I do really hate the style, it is the style clang is meant to be using)
There was a problem hiding this comment.
Oh, sorry, this is method names, not variables - I think we should match the clang style, but at this point I don't know what that actually is for method names :D
(@AaronBallman what is the correct case convention?)
There was a problem hiding this comment.
I'm not saying this is the right thing to do, but what I intended for this PR was to keep the names the same as they were in the LLVM IR codegen implementation. A few of those names were out-of-date with the latest coding style. Maybe this is a good time to update them?
There was a problem hiding this comment.
I think given this movement it is entirely reasonable to update to the current style
|
|
||
| /// Implements C++ ABI functionality that can be shared between LLVM IR codegen | ||
| /// and CIR codegen. | ||
| class CXXABIShared { |
There was a problem hiding this comment.
Nit: how about 'CXXABICommon' (or maybe just 'CXXABIBase')? IMO "Shared" might give extra room for interpretation given other idiomatic c++ stuff (e.g. shared pointers, etc)
There was a problem hiding this comment.
I would swear I had made a comment to this effect but apparently saving comments is too much to ask of myself.
I just did a quick grep of the clang tree and it looks like *Base is what is used most consistently
|
Okay, just for clarity, CIR codegen is the lowering from CIR to LLVM dialect MLIR, right? This is exactly the code that I would very strongly like to avoid duplicating between the existing CodeGen and the CIR CodeGen. While I appreciate that this PR is trying to extract all of the logic and just leave actual instruction emission for IR-specific code, I do not think this is sufficiently avoiding code duplication: the structure of the IR-specific code, knowing which calls to make and which instructions to emit, necessarily represents a lot of logic that must be duplicated. And since the information content and structure of LLVM IR and LLVM-dialect MLIR are by definition isomorphic, this duplication is essentially unnecessary — at no point in the process should we ever be making different choices based on the output IR. I think there are two reasonable paths forward here:
I have a strong preference for doing (1) in the medium term, while we're bringing up CIR CodeGen, and then tackling (2) once we've got a functional system written on top of input-generic logic. Trying to simultaneously make the logic input-generic and output-generic is going to greatly complicate the refactor and drag the entire process out. I understand that (1) interferes with the ability to do mixed-dialect MLIR lowerings where non-CIR operations are preserved in the output. However, I have not heard of there being an immediate need for that; it seems like CIR clients all want to do an arbitrary transforms (serializations, etc.) on CIR and then ultimately lower it to something in LLVM dialect. Also, it's still tractable to do this, it's just less efficient: you would just need to emit IR for an individual operation into a temporary LLVM function, raise that function into LLVM-dialect MLIR, and then splice that into your output. |
this change is something I requested specifically for the purpose of additional duplication. The specific goal of the PR is to start reducing the code duplication for the IR agnostic parts of the ABI. The existing code already has a lot of duplication, and this is just the start of of deduplication. ie this PR is reducing the duplication rather than increasing it. that said I would like to see if we can start sharing more of the IR generator - maybe a hypothetical IRBuilder interface? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
This refactors the CGCXXABI and ItaniumCXXABI classes in LLVM IR codegen and their CIR codegen counterparts so that common code can be shared between the two.
This is a small first step towards better code sharing between LLVM IR codegen and CIR codegen. We should be able to expand this significantly as CIR codegen matures.
Note that some other code can very nearly, but not quite be shared at this point. For example
AddedStructorArgsand its uses are nearly identical between LLVM IR and CIR codegen, but the struct contains anllvm::Value*member in LLVM IR codegen where it conatins anmlir::Valuemember in CIR codegen. We will need additional refactoring to handle cases like that.