-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[CIR] Introduce more cleanup infrastructure #152589
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.
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
Conversation
Support for normal cleanups was introduced with a simplified implementation compared to what's in the incubator (which corresponds closely to the classic codegen implementation). This change introduces more of the infrastructure that will later be needed to handle non-trivial cleanup cases, including exception handling.
@llvm/pr-subscribers-clang Author: Andy Kaylor (andykaylor) ChangesSupport for normal cleanups was introduced with a simplified implementation compared to what's in the incubator (which corresponds closely to the classic codegen implementation). This change introduces more of the infrastructure that will later be needed to handle non-trivial cleanup cases, including exception handling. Full diff: https://github.com/llvm/llvm-project/pull/152589.diff 4 Files Affected:
diff --git a/clang/include/clang/CIR/MissingFeatures.h b/clang/include/clang/CIR/MissingFeatures.h
index fcc8ce7caf111..926f2c57e5968 100644
--- a/clang/include/clang/CIR/MissingFeatures.h
+++ b/clang/include/clang/CIR/MissingFeatures.h
@@ -200,6 +200,8 @@ struct MissingFeatures {
static bool deferredCXXGlobalInit() { return false; }
static bool ehCleanupFlags() { return false; }
static bool ehCleanupScope() { return false; }
+ static bool ehCleanupScopeRequiresEHCleanup() { return false; }
+ static bool ehCleanupBranchFixups() { return false; }
static bool ehstackBranches() { return false; }
static bool emitCheckedInBoundsGEP() { return false; }
static bool emitCondLikelihoodViaExpectIntrinsic() { return false; }
diff --git a/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp b/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp
index b8663eb951d05..865324cb8d44f 100644
--- a/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp
@@ -70,21 +70,32 @@ void EHScopeStack::deallocate(size_t size) {
}
void *EHScopeStack::pushCleanup(CleanupKind kind, size_t size) {
- char *buffer = allocate(size);
+ char *buffer = allocate(EHCleanupScope::getSizeForCleanupSize(size));
- // When the full implementation is upstreamed, this will allocate
- // extra memory for and construct a wrapper object that is used to
- // manage the cleanup generation.
- assert(!cir::MissingFeatures::ehCleanupScope());
+ EHCleanupScope *scope = new (buffer) EHCleanupScope(size);
- return buffer;
+ return scope->getCleanupBuffer();
}
-static mlir::Block *getCurCleanupBlock(CIRGenFunction &cgf) {
- mlir::OpBuilder::InsertionGuard guard(cgf.getBuilder());
- mlir::Block *cleanup =
- cgf.curLexScope->getOrCreateCleanupBlock(cgf.getBuilder());
- return cleanup;
+void EHScopeStack::popCleanup() {
+ assert(!empty() && "popping exception stack when not empty");
+
+ assert(isa<EHCleanupScope>(*begin()));
+ EHCleanupScope &cleanup = cast<EHCleanupScope>(*begin());
+ deallocate(cleanup.getAllocatedSize());
+
+ // Destroy the cleanup.
+ cleanup.destroy();
+
+ assert(!cir::MissingFeatures::ehCleanupBranchFixups());
+}
+
+static void emitCleanup(CIRGenFunction &cgf, EHScopeStack::Cleanup *cleanup) {
+ // Ask the cleanup to emit itself.
+ assert(cgf.haveInsertPoint() && "expected insertion point");
+ assert(!cir::MissingFeatures::ehCleanupFlags());
+ cleanup->emit(cgf);
+ assert(cgf.haveInsertPoint() && "cleanup ended with no insertion point?");
}
/// Pops a cleanup block. If the block includes a normal cleanup, the
@@ -92,23 +103,56 @@ static mlir::Block *getCurCleanupBlock(CIRGenFunction &cgf) {
/// any branch fixups on the cleanup.
void CIRGenFunction::popCleanupBlock() {
assert(!ehStack.empty() && "cleanup stack is empty!");
+ assert(isa<EHCleanupScope>(*ehStack.begin()) && "top not a cleanup!");
+ EHCleanupScope &scope = cast<EHCleanupScope>(*ehStack.begin());
- // The memory for the cleanup continues to be owned by the EHScopeStack
- // allocator, so we just destroy the object rather than attempting to
- // free it.
- EHScopeStack::Cleanup &cleanup = *ehStack.begin();
+ // Remember activation information.
+ bool isActive = scope.isActive();
- // The eventual implementation here will use the EHCleanupScope helper class.
- assert(!cir::MissingFeatures::ehCleanupScope());
+ assert(!cir::MissingFeatures::ehCleanupBranchFixups());
- mlir::OpBuilder::InsertionGuard guard(builder);
+ // - whether there's a fallthrough
+ mlir::Block *fallthroughSource = builder.getInsertionBlock();
+ bool hasFallthrough = fallthroughSource != nullptr && isActive;
+
+ bool requiresNormalCleanup = scope.isNormalCleanup() && hasFallthrough;
+
+ // If we don't need the cleanup at all, we're done.
+ assert(!cir::MissingFeatures::ehCleanupScopeRequiresEHCleanup());
+ if (!requiresNormalCleanup) {
+ ehStack.popCleanup();
+ return;
+ }
+
+ // Copy the cleanup emission data out. This uses either a stack
+ // array or malloc'd memory, depending on the size, which is
+ // behavior that SmallVector would provide, if we could use it
+ // here. Unfortunately, if you ask for a SmallVector<char>, the
+ // alignment isn't sufficient.
+ auto *cleanupSource = reinterpret_cast<char *>(scope.getCleanupBuffer());
+ alignas(EHScopeStack::ScopeStackAlignment) char
+ cleanupBufferStack[8 * sizeof(void *)];
+ std::unique_ptr<char[]> cleanupBufferHeap;
+ size_t cleanupSize = scope.getCleanupSize();
+ EHScopeStack::Cleanup *cleanup;
+
+ // This is necessary because we are going to deallocate the cleanup
+ // (in popCleanup) before we emit it.
+ if (cleanupSize <= sizeof(cleanupBufferStack)) {
+ memcpy(cleanupBufferStack, cleanupSource, cleanupSize);
+ cleanup = reinterpret_cast<EHScopeStack::Cleanup *>(cleanupBufferStack);
+ } else {
+ cleanupBufferHeap.reset(new char[cleanupSize]);
+ memcpy(cleanupBufferHeap.get(), cleanupSource, cleanupSize);
+ cleanup =
+ reinterpret_cast<EHScopeStack::Cleanup *>(cleanupBufferHeap.get());
+ }
assert(!cir::MissingFeatures::ehCleanupFlags());
- mlir::Block *cleanupEntry = getCurCleanupBlock(*this);
- builder.setInsertionPointToEnd(cleanupEntry);
- cleanup.emit(*this);
- ehStack.deallocate(cleanup.getSize());
+ ehStack.popCleanup();
+ scope.markEmitted();
+ emitCleanup(*this, cleanup);
}
/// Pops cleanup blocks until the given savepoint is reached.
diff --git a/clang/lib/CIR/CodeGen/CIRGenCleanup.h b/clang/lib/CIR/CodeGen/CIRGenCleanup.h
index 7361c8c00891b..0de228993a630 100644
--- a/clang/lib/CIR/CodeGen/CIRGenCleanup.h
+++ b/clang/lib/CIR/CodeGen/CIRGenCleanup.h
@@ -14,10 +14,117 @@
#ifndef CLANG_LIB_CIR_CODEGEN_CIRGENCLEANUP_H
#define CLANG_LIB_CIR_CODEGEN_CIRGENCLEANUP_H
+#include "Address.h"
#include "EHScopeStack.h"
+#include "mlir/IR/Value.h"
namespace clang::CIRGen {
+/// A protected scope for zero-cost EH handling.
+class EHScope {
+ class CommonBitFields {
+ friend class EHScope;
+ unsigned kind : 3;
+ };
+ enum { NumCommonBits = 3 };
+
+protected:
+ class CleanupBitFields {
+ friend class EHCleanupScope;
+ unsigned : NumCommonBits;
+
+ /// Whether this cleanup needs to be run along normal edges.
+ unsigned isNormalCleanup : 1;
+
+ /// Whether this cleanup needs to be run along exception edges.
+ unsigned isEHCleanup : 1;
+
+ /// Whether this cleanup is currently active.
+ unsigned isActive : 1;
+
+ /// Whether this cleanup is a lifetime marker
+ unsigned isLifetimeMarker : 1;
+
+ /// Whether the normal cleanup should test the activation flag.
+ unsigned testFlagInNormalCleanup : 1;
+
+ /// Whether the EH cleanup should test the activation flag.
+ unsigned testFlagInEHCleanup : 1;
+
+ /// The amount of extra storage needed by the Cleanup.
+ /// Always a multiple of the scope-stack alignment.
+ unsigned cleanupSize : 12;
+ };
+
+ union {
+ CommonBitFields commonBits;
+ CleanupBitFields cleanupBits;
+ };
+
+public:
+ enum Kind { Cleanup, Catch, Terminate, Filter };
+
+ EHScope(Kind kind) { commonBits.kind = kind; }
+
+ Kind getKind() const { return static_cast<Kind>(commonBits.kind); }
+};
+
+/// A cleanup scope which generates the cleanup blocks lazily.
+class alignas(8) EHCleanupScope : public EHScope {
+public:
+ /// Gets the size required for a lazy cleanup scope with the given
+ /// cleanup-data requirements.
+ static size_t getSizeForCleanupSize(size_t size) {
+ return sizeof(EHCleanupScope) + size;
+ }
+
+ size_t getAllocatedSize() const {
+ return sizeof(EHCleanupScope) + cleanupBits.cleanupSize;
+ }
+
+ EHCleanupScope(unsigned cleanupSize) : EHScope(EHScope::Cleanup) {
+ // TODO(cir): When exception handling is upstreamed, isNormalCleanup and
+ // isEHCleanup will be arguments to the constructor.
+ cleanupBits.isNormalCleanup = true;
+ cleanupBits.isEHCleanup = false;
+ cleanupBits.isActive = true;
+ cleanupBits.isLifetimeMarker = false;
+ cleanupBits.testFlagInNormalCleanup = false;
+ cleanupBits.testFlagInEHCleanup = false;
+ cleanupBits.cleanupSize = cleanupSize;
+
+ assert(cleanupBits.cleanupSize == cleanupSize && "cleanup size overflow");
+ }
+
+ void destroy() {}
+ // Objects of EHCleanupScope are not destructed. Use destroy().
+ ~EHCleanupScope() = delete;
+
+ bool isNormalCleanup() const { return cleanupBits.isNormalCleanup; }
+
+ bool isActive() const { return cleanupBits.isActive; }
+
+ size_t getCleanupSize() const { return cleanupBits.cleanupSize; }
+ void *getCleanupBuffer() { return this + 1; }
+
+ EHScopeStack::Cleanup *getCleanup() {
+ return reinterpret_cast<EHScopeStack::Cleanup *>(getCleanupBuffer());
+ }
+
+ static bool classof(const EHScope *scope) {
+ return (scope->getKind() == Cleanup);
+ }
+
+ void markEmitted() {}
+};
+// NOTE: There can be additional data classes tacked on after an EHCleanupScope.
+// It is asserted (in EHScopeStack::pushCleanup*) that they don't require
+// greater alignment than ScopeStackAlignment. So, EHCleanupScope must have
+// alignment equal to that -- not more (would be misaligned by the stack
+// allocator), and not less (would break the appended classes).
+static_assert(alignof(EHCleanupScope) == EHScopeStack::ScopeStackAlignment,
+ "EHCleanupScope expected alignment");
+
/// A non-stable pointer into the scope stack.
class EHScopeStack::iterator {
char *ptr = nullptr;
@@ -28,11 +135,9 @@ class EHScopeStack::iterator {
public:
iterator() = default;
- EHScopeStack::Cleanup *get() const {
- return reinterpret_cast<EHScopeStack::Cleanup *>(ptr);
- }
+ EHScope *get() const { return reinterpret_cast<EHScope *>(ptr); }
- EHScopeStack::Cleanup &operator*() const { return *get(); }
+ EHScope &operator*() const { return *get(); }
};
inline EHScopeStack::iterator EHScopeStack::begin() const {
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.h b/clang/lib/CIR/CodeGen/CIRGenFunction.h
index bdbc77c0a7c4a..ccb9fb1cc7840 100644
--- a/clang/lib/CIR/CodeGen/CIRGenFunction.h
+++ b/clang/lib/CIR/CodeGen/CIRGenFunction.h
@@ -337,6 +337,15 @@ class CIRGenFunction : public CIRGenTypeCache {
const clang::LangOptions &getLangOpts() const { return cgm.getLangOpts(); }
+ /// True if an insertion point is defined. If not, this indicates that the
+ /// current code being emitted is unreachable.
+ /// FIXME(cir): we need to inspect this and perhaps use a cleaner mechanism
+ /// since we don't yet force null insertion point to designate behavior (like
+ /// LLVM's codegen does) and we probably shouldn't.
+ bool haveInsertPoint() const {
+ return builder.getInsertionBlock() != nullptr;
+ }
+
// Wrapper for function prototype sources. Wraps either a FunctionProtoType or
// an ObjCMethodDecl.
struct PrototypeWrapper {
|
@llvm/pr-subscribers-clangir Author: Andy Kaylor (andykaylor) ChangesSupport for normal cleanups was introduced with a simplified implementation compared to what's in the incubator (which corresponds closely to the classic codegen implementation). This change introduces more of the infrastructure that will later be needed to handle non-trivial cleanup cases, including exception handling. Full diff: https://github.com/llvm/llvm-project/pull/152589.diff 4 Files Affected:
diff --git a/clang/include/clang/CIR/MissingFeatures.h b/clang/include/clang/CIR/MissingFeatures.h
index fcc8ce7caf111..926f2c57e5968 100644
--- a/clang/include/clang/CIR/MissingFeatures.h
+++ b/clang/include/clang/CIR/MissingFeatures.h
@@ -200,6 +200,8 @@ struct MissingFeatures {
static bool deferredCXXGlobalInit() { return false; }
static bool ehCleanupFlags() { return false; }
static bool ehCleanupScope() { return false; }
+ static bool ehCleanupScopeRequiresEHCleanup() { return false; }
+ static bool ehCleanupBranchFixups() { return false; }
static bool ehstackBranches() { return false; }
static bool emitCheckedInBoundsGEP() { return false; }
static bool emitCondLikelihoodViaExpectIntrinsic() { return false; }
diff --git a/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp b/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp
index b8663eb951d05..865324cb8d44f 100644
--- a/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp
@@ -70,21 +70,32 @@ void EHScopeStack::deallocate(size_t size) {
}
void *EHScopeStack::pushCleanup(CleanupKind kind, size_t size) {
- char *buffer = allocate(size);
+ char *buffer = allocate(EHCleanupScope::getSizeForCleanupSize(size));
- // When the full implementation is upstreamed, this will allocate
- // extra memory for and construct a wrapper object that is used to
- // manage the cleanup generation.
- assert(!cir::MissingFeatures::ehCleanupScope());
+ EHCleanupScope *scope = new (buffer) EHCleanupScope(size);
- return buffer;
+ return scope->getCleanupBuffer();
}
-static mlir::Block *getCurCleanupBlock(CIRGenFunction &cgf) {
- mlir::OpBuilder::InsertionGuard guard(cgf.getBuilder());
- mlir::Block *cleanup =
- cgf.curLexScope->getOrCreateCleanupBlock(cgf.getBuilder());
- return cleanup;
+void EHScopeStack::popCleanup() {
+ assert(!empty() && "popping exception stack when not empty");
+
+ assert(isa<EHCleanupScope>(*begin()));
+ EHCleanupScope &cleanup = cast<EHCleanupScope>(*begin());
+ deallocate(cleanup.getAllocatedSize());
+
+ // Destroy the cleanup.
+ cleanup.destroy();
+
+ assert(!cir::MissingFeatures::ehCleanupBranchFixups());
+}
+
+static void emitCleanup(CIRGenFunction &cgf, EHScopeStack::Cleanup *cleanup) {
+ // Ask the cleanup to emit itself.
+ assert(cgf.haveInsertPoint() && "expected insertion point");
+ assert(!cir::MissingFeatures::ehCleanupFlags());
+ cleanup->emit(cgf);
+ assert(cgf.haveInsertPoint() && "cleanup ended with no insertion point?");
}
/// Pops a cleanup block. If the block includes a normal cleanup, the
@@ -92,23 +103,56 @@ static mlir::Block *getCurCleanupBlock(CIRGenFunction &cgf) {
/// any branch fixups on the cleanup.
void CIRGenFunction::popCleanupBlock() {
assert(!ehStack.empty() && "cleanup stack is empty!");
+ assert(isa<EHCleanupScope>(*ehStack.begin()) && "top not a cleanup!");
+ EHCleanupScope &scope = cast<EHCleanupScope>(*ehStack.begin());
- // The memory for the cleanup continues to be owned by the EHScopeStack
- // allocator, so we just destroy the object rather than attempting to
- // free it.
- EHScopeStack::Cleanup &cleanup = *ehStack.begin();
+ // Remember activation information.
+ bool isActive = scope.isActive();
- // The eventual implementation here will use the EHCleanupScope helper class.
- assert(!cir::MissingFeatures::ehCleanupScope());
+ assert(!cir::MissingFeatures::ehCleanupBranchFixups());
- mlir::OpBuilder::InsertionGuard guard(builder);
+ // - whether there's a fallthrough
+ mlir::Block *fallthroughSource = builder.getInsertionBlock();
+ bool hasFallthrough = fallthroughSource != nullptr && isActive;
+
+ bool requiresNormalCleanup = scope.isNormalCleanup() && hasFallthrough;
+
+ // If we don't need the cleanup at all, we're done.
+ assert(!cir::MissingFeatures::ehCleanupScopeRequiresEHCleanup());
+ if (!requiresNormalCleanup) {
+ ehStack.popCleanup();
+ return;
+ }
+
+ // Copy the cleanup emission data out. This uses either a stack
+ // array or malloc'd memory, depending on the size, which is
+ // behavior that SmallVector would provide, if we could use it
+ // here. Unfortunately, if you ask for a SmallVector<char>, the
+ // alignment isn't sufficient.
+ auto *cleanupSource = reinterpret_cast<char *>(scope.getCleanupBuffer());
+ alignas(EHScopeStack::ScopeStackAlignment) char
+ cleanupBufferStack[8 * sizeof(void *)];
+ std::unique_ptr<char[]> cleanupBufferHeap;
+ size_t cleanupSize = scope.getCleanupSize();
+ EHScopeStack::Cleanup *cleanup;
+
+ // This is necessary because we are going to deallocate the cleanup
+ // (in popCleanup) before we emit it.
+ if (cleanupSize <= sizeof(cleanupBufferStack)) {
+ memcpy(cleanupBufferStack, cleanupSource, cleanupSize);
+ cleanup = reinterpret_cast<EHScopeStack::Cleanup *>(cleanupBufferStack);
+ } else {
+ cleanupBufferHeap.reset(new char[cleanupSize]);
+ memcpy(cleanupBufferHeap.get(), cleanupSource, cleanupSize);
+ cleanup =
+ reinterpret_cast<EHScopeStack::Cleanup *>(cleanupBufferHeap.get());
+ }
assert(!cir::MissingFeatures::ehCleanupFlags());
- mlir::Block *cleanupEntry = getCurCleanupBlock(*this);
- builder.setInsertionPointToEnd(cleanupEntry);
- cleanup.emit(*this);
- ehStack.deallocate(cleanup.getSize());
+ ehStack.popCleanup();
+ scope.markEmitted();
+ emitCleanup(*this, cleanup);
}
/// Pops cleanup blocks until the given savepoint is reached.
diff --git a/clang/lib/CIR/CodeGen/CIRGenCleanup.h b/clang/lib/CIR/CodeGen/CIRGenCleanup.h
index 7361c8c00891b..0de228993a630 100644
--- a/clang/lib/CIR/CodeGen/CIRGenCleanup.h
+++ b/clang/lib/CIR/CodeGen/CIRGenCleanup.h
@@ -14,10 +14,117 @@
#ifndef CLANG_LIB_CIR_CODEGEN_CIRGENCLEANUP_H
#define CLANG_LIB_CIR_CODEGEN_CIRGENCLEANUP_H
+#include "Address.h"
#include "EHScopeStack.h"
+#include "mlir/IR/Value.h"
namespace clang::CIRGen {
+/// A protected scope for zero-cost EH handling.
+class EHScope {
+ class CommonBitFields {
+ friend class EHScope;
+ unsigned kind : 3;
+ };
+ enum { NumCommonBits = 3 };
+
+protected:
+ class CleanupBitFields {
+ friend class EHCleanupScope;
+ unsigned : NumCommonBits;
+
+ /// Whether this cleanup needs to be run along normal edges.
+ unsigned isNormalCleanup : 1;
+
+ /// Whether this cleanup needs to be run along exception edges.
+ unsigned isEHCleanup : 1;
+
+ /// Whether this cleanup is currently active.
+ unsigned isActive : 1;
+
+ /// Whether this cleanup is a lifetime marker
+ unsigned isLifetimeMarker : 1;
+
+ /// Whether the normal cleanup should test the activation flag.
+ unsigned testFlagInNormalCleanup : 1;
+
+ /// Whether the EH cleanup should test the activation flag.
+ unsigned testFlagInEHCleanup : 1;
+
+ /// The amount of extra storage needed by the Cleanup.
+ /// Always a multiple of the scope-stack alignment.
+ unsigned cleanupSize : 12;
+ };
+
+ union {
+ CommonBitFields commonBits;
+ CleanupBitFields cleanupBits;
+ };
+
+public:
+ enum Kind { Cleanup, Catch, Terminate, Filter };
+
+ EHScope(Kind kind) { commonBits.kind = kind; }
+
+ Kind getKind() const { return static_cast<Kind>(commonBits.kind); }
+};
+
+/// A cleanup scope which generates the cleanup blocks lazily.
+class alignas(8) EHCleanupScope : public EHScope {
+public:
+ /// Gets the size required for a lazy cleanup scope with the given
+ /// cleanup-data requirements.
+ static size_t getSizeForCleanupSize(size_t size) {
+ return sizeof(EHCleanupScope) + size;
+ }
+
+ size_t getAllocatedSize() const {
+ return sizeof(EHCleanupScope) + cleanupBits.cleanupSize;
+ }
+
+ EHCleanupScope(unsigned cleanupSize) : EHScope(EHScope::Cleanup) {
+ // TODO(cir): When exception handling is upstreamed, isNormalCleanup and
+ // isEHCleanup will be arguments to the constructor.
+ cleanupBits.isNormalCleanup = true;
+ cleanupBits.isEHCleanup = false;
+ cleanupBits.isActive = true;
+ cleanupBits.isLifetimeMarker = false;
+ cleanupBits.testFlagInNormalCleanup = false;
+ cleanupBits.testFlagInEHCleanup = false;
+ cleanupBits.cleanupSize = cleanupSize;
+
+ assert(cleanupBits.cleanupSize == cleanupSize && "cleanup size overflow");
+ }
+
+ void destroy() {}
+ // Objects of EHCleanupScope are not destructed. Use destroy().
+ ~EHCleanupScope() = delete;
+
+ bool isNormalCleanup() const { return cleanupBits.isNormalCleanup; }
+
+ bool isActive() const { return cleanupBits.isActive; }
+
+ size_t getCleanupSize() const { return cleanupBits.cleanupSize; }
+ void *getCleanupBuffer() { return this + 1; }
+
+ EHScopeStack::Cleanup *getCleanup() {
+ return reinterpret_cast<EHScopeStack::Cleanup *>(getCleanupBuffer());
+ }
+
+ static bool classof(const EHScope *scope) {
+ return (scope->getKind() == Cleanup);
+ }
+
+ void markEmitted() {}
+};
+// NOTE: There can be additional data classes tacked on after an EHCleanupScope.
+// It is asserted (in EHScopeStack::pushCleanup*) that they don't require
+// greater alignment than ScopeStackAlignment. So, EHCleanupScope must have
+// alignment equal to that -- not more (would be misaligned by the stack
+// allocator), and not less (would break the appended classes).
+static_assert(alignof(EHCleanupScope) == EHScopeStack::ScopeStackAlignment,
+ "EHCleanupScope expected alignment");
+
/// A non-stable pointer into the scope stack.
class EHScopeStack::iterator {
char *ptr = nullptr;
@@ -28,11 +135,9 @@ class EHScopeStack::iterator {
public:
iterator() = default;
- EHScopeStack::Cleanup *get() const {
- return reinterpret_cast<EHScopeStack::Cleanup *>(ptr);
- }
+ EHScope *get() const { return reinterpret_cast<EHScope *>(ptr); }
- EHScopeStack::Cleanup &operator*() const { return *get(); }
+ EHScope &operator*() const { return *get(); }
};
inline EHScopeStack::iterator EHScopeStack::begin() const {
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.h b/clang/lib/CIR/CodeGen/CIRGenFunction.h
index bdbc77c0a7c4a..ccb9fb1cc7840 100644
--- a/clang/lib/CIR/CodeGen/CIRGenFunction.h
+++ b/clang/lib/CIR/CodeGen/CIRGenFunction.h
@@ -337,6 +337,15 @@ class CIRGenFunction : public CIRGenTypeCache {
const clang::LangOptions &getLangOpts() const { return cgm.getLangOpts(); }
+ /// True if an insertion point is defined. If not, this indicates that the
+ /// current code being emitted is unreachable.
+ /// FIXME(cir): we need to inspect this and perhaps use a cleaner mechanism
+ /// since we don't yet force null insertion point to designate behavior (like
+ /// LLVM's codegen does) and we probably shouldn't.
+ bool haveInsertPoint() const {
+ return builder.getInsertionBlock() != nullptr;
+ }
+
// Wrapper for function prototype sources. Wraps either a FunctionProtoType or
// an ObjCMethodDecl.
struct PrototypeWrapper {
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a few minor comments.
// extra memory for and construct a wrapper object that is used to | ||
// manage the cleanup generation. | ||
assert(!cir::MissingFeatures::ehCleanupScope()); | ||
EHCleanupScope *scope = new (buffer) EHCleanupScope(size); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a errorNYI("SEH")
?
Note that I just asked the same thing here: #152802 (comment)
// Copy the cleanup emission data out. This uses either a stack | ||
// array or malloc'd memory, depending on the size, which is | ||
// behavior that SmallVector would provide, if we could use it | ||
// here. Unfortunately, if you ask for a SmallVector<char>, the | ||
// alignment isn't sufficient. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to do an aligned operator new below now that we can use C++17.
But isn't it easier to create a SmallVector<EHScopeStack::Cleanup>
and cast vector.data()
to char *
than the other way around? This way we get our alignment requrement satisfied, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, this part of the code really concerns me. We've gone to a lot of effort to have an ultra-efficient allocator because cleanups are being pushed and popped from the stack so frequently, but then every time we pop a cleanup we're copying the memory here. It seems counter-productive. I'm just reluctant to take on a major change to this code because it's been working in classic codegen for a long time.
The goal here is to grab a small bit of space on the heap. Until this change six years ago, it was using llvm::AlignedCharArray. We can't use SmallVector<EHScopeStack::Cleanup>
because there are potentially other objects tacked on to the end of the cleanup. That's why this is using a char-based allocation and a size. I think the heap space will be used in almost all cases, and I'm concerned that anything else could introduce compile time problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just reluctant to take on a major change to this code because it's been working in classic codegen for a long time
+1, if such changes have to happen it might be worth do a separate PR that handles changes in both OG and CIR.
}; | ||
|
||
/// A cleanup scope which generates the cleanup blocks lazily. | ||
class alignas(8) EHCleanupScope : public EHScope { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be alignas(EHScopeStack::ScopeStackAlignment)
? We can get rid of that static_assert below the class declaration this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Support for normal cleanups was introduced with a simplified implementation compared to what's in the incubator (which corresponds closely to the classic codegen implementation).
This change introduces more of the infrastructure that will later be needed to handle non-trivial cleanup cases, including exception handling.