[CIR] Refactor global variable emission and initialization #138222
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
When global variable support was initially upstreamed, we took some shortcuts and only implemented the minimum support for simple variables and constant initializers. This change refactors the code that creates global variables to introduce more of the complexities that are present in the incubator and the classic codegen. I can't really say this is NFC, because the code executed is very different and it will report different NYI diagnostics, but for the currently implemented cases, it results in the same output.
andykaylor
requested review from
xlauko,
bcardosolopes,
mmha,
erichkeane,
lanza and
AmrDeveloper
llvmbot
added
clang
|
@llvm/pr-subscribers-clangir @llvm/pr-subscribers-clang Author: Andy Kaylor (andykaylor) ChangesWhen global variable support was initially upstreamed, we took some shortcuts and only implemented the minimum support for simple variables and constant initializers. This change refactors the code that creates global variables to introduce more of the complexities that are present in the incubator and the classic codegen. I can't really say this is NFC, because the code executed is very different and it will report different NYI diagnostics, but for the currently implemented cases, it results in the same output. Full diff: https://github.com/llvm/llvm-project/pull/138222.diff 5 Files Affected:
diff --git a/clang/include/clang/CIR/MissingFeatures.h b/clang/include/clang/CIR/MissingFeatures.h
index 4d4951aa0e126..3a6b1874581e7 100644
--- a/clang/include/clang/CIR/MissingFeatures.h
+++ b/clang/include/clang/CIR/MissingFeatures.h
@@ -36,6 +36,8 @@ struct MissingFeatures {
static bool opGlobalConstant() { return false; }
static bool opGlobalAlignment() { return false; }
static bool opGlobalWeakRef() { return false; }
+ static bool opGlobalLinkage() { return false; }
+ static bool opGlobalSetVisitibility() { return false; }
static bool supportIFuncAttr() { return false; }
static bool supportVisibility() { return false; }
@@ -163,6 +165,10 @@ struct MissingFeatures {
static bool setDSOLocal() { return false; }
static bool foldCaseStmt() { return false; }
static bool constantFoldSwitchStatement() { return false; }
+ static bool cudaSupport() { return false; }
+ static bool maybeHandleStaticInExternC() { return false; }
+ static bool constEmitterArrayILE() { return false; }
+ static bool constEmitterVectorILE() { return false; }
// Missing types
static bool dataMemberType() { return false; }
diff --git a/clang/lib/CIR/CodeGen/CIRGenConstantEmitter.h b/clang/lib/CIR/CodeGen/CIRGenConstantEmitter.h
index ca4e607992bbc..54c3710690ef1 100644
--- a/clang/lib/CIR/CodeGen/CIRGenConstantEmitter.h
+++ b/clang/lib/CIR/CodeGen/CIRGenConstantEmitter.h
@@ -31,6 +31,15 @@ class ConstantEmitter {
private:
bool abstract = false;
+ /// Whether non-abstract components of the emitter have been initialized.
+ bool initializedNonAbstract = false;
+
+ /// Whether the emitter has been finalized.
+ bool finalized = false;
+
+ /// Whether the constant-emission failed.
+ bool failed = false;
+
/// Whether we're in a constant context.
bool inConstantContext = false;
@@ -46,6 +55,14 @@ class ConstantEmitter {
ConstantEmitter(const ConstantEmitter &other) = delete;
ConstantEmitter &operator=(const ConstantEmitter &other) = delete;
+ ~ConstantEmitter();
+
+ /// Try to emit the initiaizer of the given declaration as an abstract
+ /// constant. If this succeeds, the emission must be finalized.
+ mlir::Attribute tryEmitForInitializer(const VarDecl &d);
+
+ void finalize(cir::GlobalOp gv);
+
// All of the "abstract" emission methods below permit the emission to
// be immediately discarded without finalizing anything. Therefore, they
// must also promise not to do anything that will, in the future, require
@@ -61,6 +78,10 @@ class ConstantEmitter {
// reference to its current location.
mlir::Attribute emitForMemory(mlir::Attribute c, QualType t);
+ /// Try to emit the initializer of the given declaration as an abstract
+ /// constant.
+ mlir::Attribute tryEmitAbstractForInitializer(const VarDecl &d);
+
/// Emit the result of the given expression as an abstract constant,
/// asserting that it succeeded. This is only safe to do when the
/// expression is known to be a constant expression with either a fairly
@@ -79,11 +100,18 @@ class ConstantEmitter {
mlir::Attribute tryEmitPrivate(const APValue &value, QualType destType);
mlir::Attribute tryEmitPrivateForMemory(const APValue &value, QualType t);
- /// Try to emit the initializer of the given declaration as an abstract
- /// constant.
- mlir::Attribute tryEmitAbstractForInitializer(const VarDecl &d);
-
private:
+ void initializeNonAbstract() {
+ assert(!initializedNonAbstract);
+ initializedNonAbstract = true;
+ assert(!cir::MissingFeatures::addressSpace());
+ }
+ mlir::Attribute markIfFailed(mlir::Attribute init) {
+ if (!init)
+ failed = true;
+ return init;
+ }
+
class AbstractStateRAII {
ConstantEmitter &emitter;
bool oldValue;
diff --git a/clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp b/clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp
index 6e5c7b8fb51f8..ca718677d7442 100644
--- a/clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp
@@ -90,8 +90,100 @@ class ConstExprEmitter
}
mlir::Attribute VisitCastExpr(CastExpr *e, QualType destType) {
- cgm.errorNYI(e->getBeginLoc(), "ConstExprEmitter::VisitCastExpr");
- return {};
+ if (const auto *ece = dyn_cast<ExplicitCastExpr>(e))
+ cgm.errorNYI(e->getBeginLoc(),
+ "ConstExprEmitter::VisitCastExpr explicit cast");
+ Expr *subExpr = e->getSubExpr();
+
+ switch (e->getCastKind()) {
+ case CK_HLSLArrayRValue:
+ case CK_HLSLVectorTruncation:
+ case CK_HLSLElementwiseCast:
+ case CK_HLSLAggregateSplatCast:
+ case CK_ToUnion:
+ case CK_AddressSpaceConversion:
+ case CK_ReinterpretMemberPointer:
+ case CK_DerivedToBaseMemberPointer:
+ case CK_BaseToDerivedMemberPointer:
+ cgm.errorNYI(e->getBeginLoc(), "ConstExprEmitter::VisitCastExpr");
+ return {};
+
+ case CK_LValueToRValue:
+ case CK_AtomicToNonAtomic:
+ case CK_NonAtomicToAtomic:
+ case CK_NoOp:
+ case CK_ConstructorConversion:
+ return Visit(subExpr, destType);
+
+ case CK_IntToOCLSampler:
+ llvm_unreachable("global sampler variables are not generated");
+
+ case CK_Dependent:
+ llvm_unreachable("saw dependent cast!");
+
+ case CK_BuiltinFnToFnPtr:
+ llvm_unreachable("builtin functions are handled elsewhere");
+
+ // These will never be supported.
+ case CK_ObjCObjectLValueCast:
+ case CK_ARCProduceObject:
+ case CK_ARCConsumeObject:
+ case CK_ARCReclaimReturnedObject:
+ case CK_ARCExtendBlockObject:
+ case CK_CopyAndAutoreleaseBlockObject:
+ return {};
+
+ // These don't need to be handled here because Evaluate knows how to
+ // evaluate them in the cases where they can be folded.
+ case CK_BitCast:
+ case CK_ToVoid:
+ case CK_Dynamic:
+ case CK_LValueBitCast:
+ case CK_LValueToRValueBitCast:
+ case CK_NullToMemberPointer:
+ case CK_UserDefinedConversion:
+ case CK_CPointerToObjCPointerCast:
+ case CK_BlockPointerToObjCPointerCast:
+ case CK_AnyPointerToBlockPointerCast:
+ case CK_ArrayToPointerDecay:
+ case CK_FunctionToPointerDecay:
+ case CK_BaseToDerived:
+ case CK_DerivedToBase:
+ case CK_UncheckedDerivedToBase:
+ case CK_MemberPointerToBoolean:
+ case CK_VectorSplat:
+ case CK_FloatingRealToComplex:
+ case CK_FloatingComplexToReal:
+ case CK_FloatingComplexToBoolean:
+ case CK_FloatingComplexCast:
+ case CK_FloatingComplexToIntegralComplex:
+ case CK_IntegralRealToComplex:
+ case CK_IntegralComplexToReal:
+ case CK_IntegralComplexToBoolean:
+ case CK_IntegralComplexCast:
+ case CK_IntegralComplexToFloatingComplex:
+ case CK_PointerToIntegral:
+ case CK_PointerToBoolean:
+ case CK_NullToPointer:
+ case CK_IntegralCast:
+ case CK_BooleanToSignedIntegral:
+ case CK_IntegralToPointer:
+ case CK_IntegralToBoolean:
+ case CK_IntegralToFloating:
+ case CK_FloatingToIntegral:
+ case CK_FloatingToBoolean:
+ case CK_FloatingCast:
+ case CK_FloatingToFixedPoint:
+ case CK_FixedPointToFloating:
+ case CK_FixedPointCast:
+ case CK_FixedPointToBoolean:
+ case CK_FixedPointToIntegral:
+ case CK_IntegralToFixedPoint:
+ case CK_ZeroToOCLOpaqueType:
+ case CK_MatrixCast:
+ return {};
+ }
+ llvm_unreachable("Invalid CastKind");
}
mlir::Attribute VisitCXXDefaultInitExpr(CXXDefaultInitExpr *die, QualType t) {
@@ -118,7 +210,26 @@ class ConstExprEmitter
}
mlir::Attribute VisitInitListExpr(InitListExpr *ile, QualType t) {
- cgm.errorNYI(ile->getBeginLoc(), "ConstExprEmitter::VisitInitListExpr");
+ if (ile->isTransparent())
+ return Visit(ile->getInit(0), t);
+
+ if (ile->getType()->isArrayType()) {
+ // If we return null here, the non-constant initializer will take care of
+ // it, but we would prefer to handle it here.
+ assert(!cir::MissingFeatures::constEmitterArrayILE());
+ return {};
+ }
+
+ if (ile->getType()->isRecordType())
+ cgm.errorNYI(ile->getBeginLoc(), "ConstExprEmitter: record ILE");
+
+ if (ile->getType()->isVectorType()) {
+ // If we return null here, the non-constant initializer will take care of
+ // it, but we would prefer to handle it here.
+ assert(!cir::MissingFeatures::constEmitterVectorILE());
+ return {};
+ }
+
return {};
}
@@ -218,12 +329,32 @@ emitArrayConstant(CIRGenModule &cgm, mlir::Type desiredType,
// ConstantEmitter
//===----------------------------------------------------------------------===//
+mlir::Attribute ConstantEmitter::tryEmitForInitializer(const VarDecl &d) {
+ initializeNonAbstract();
+ return markIfFailed(tryEmitPrivateForVarInit(d));
+}
+
+void ConstantEmitter::finalize(cir::GlobalOp gv) {
+ assert(initializedNonAbstract &&
+ "finalizing emitter that was used for abstract emission?");
+ assert(!finalized && "finalizing emitter multiple times");
+ assert(!gv.isDeclaration());
+
+ // Note that we might also be Failed.
+ finalized = true;
+}
+
mlir::Attribute
ConstantEmitter::tryEmitAbstractForInitializer(const VarDecl &d) {
AbstractStateRAII state(*this, true);
return tryEmitPrivateForVarInit(d);
}
+ConstantEmitter::~ConstantEmitter() {
+ assert((!initializedNonAbstract || finalized || failed) &&
+ "not finalized after being initialized for non-abstract emission");
+}
+
mlir::Attribute ConstantEmitter::tryEmitPrivateForVarInit(const VarDecl &d) {
// Make a quick check if variable can be default NULL initialized
// and avoid going through rest of code which may do, for c++11,
diff --git a/clang/lib/CIR/CodeGen/CIRGenModule.cpp b/clang/lib/CIR/CodeGen/CIRGenModule.cpp
index a6a4330ff2ce1..a13ee5c956436 100644
--- a/clang/lib/CIR/CodeGen/CIRGenModule.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenModule.cpp
@@ -269,6 +269,40 @@ mlir::Operation *CIRGenModule::getGlobalValue(StringRef name) {
return mlir::SymbolTable::lookupSymbolIn(theModule, name);
}
+cir::GlobalOp CIRGenModule::createGlobalOp(CIRGenModule &cgm,
+ mlir::Location loc, StringRef name,
+ mlir::Type t,
+ mlir::Operation *insertPoint) {
+ cir::GlobalOp g;
+ CIRGenBuilderTy &builder = cgm.getBuilder();
+
+ {
+ mlir::OpBuilder::InsertionGuard guard(builder);
+
+ // Some global emissions are triggered while emitting a function, e.g.
+ // void s() { const char *s = "yolo"; ... }
+ //
+ // Be sure to insert global before the current function
+ CIRGenFunction *curCGF = cgm.curCGF;
+ if (curCGF)
+ builder.setInsertionPoint(curCGF->curFn);
+
+ g = builder.create<cir::GlobalOp>(loc, name, t);
+ if (!curCGF) {
+ if (insertPoint)
+ cgm.getModule().insert(insertPoint, g);
+ else
+ cgm.getModule().push_back(g);
+ }
+
+ // Default to private until we can judge based on the initializer,
+ // since MLIR doesn't allow public declarations.
+ mlir::SymbolTable::setSymbolVisibility(
+ g, mlir::SymbolTable::Visibility::Private);
+ }
+ return g;
+}
+
/// If the specified mangled name is not in the module,
/// create and return an mlir GlobalOp with the specified type (TODO(cir):
/// address space).
@@ -324,8 +358,16 @@ CIRGenModule::getOrCreateCIRGlobal(StringRef mangledName, mlir::Type ty,
return entry;
}
- errorNYI(d->getSourceRange(), "reference of undeclared global");
- return {};
+ mlir::Location loc = getLoc(d->getSourceRange());
+
+ // mlir::SymbolTable::Visibility::Public is the default, no need to explicitly
+ // mark it as such.
+ cir::GlobalOp gv =
+ CIRGenModule::createGlobalOp(*this, loc, mangledName, ty,
+ /*insertPoint=*/entry.getOperation());
+
+ // errorNYI(d->getSourceRange(), "reference of undeclared global");
+ return gv;
}
cir::GlobalOp
@@ -365,46 +407,108 @@ mlir::Value CIRGenModule::getAddrOfGlobalVar(const VarDecl *d, mlir::Type ty,
void CIRGenModule::emitGlobalVarDefinition(const clang::VarDecl *vd,
bool isTentative) {
const QualType astTy = vd->getType();
- const mlir::Type type = convertType(vd->getType());
- if (vd->getIdentifier()) {
- StringRef name = getMangledName(GlobalDecl(vd));
- auto varOp =
- builder.create<cir::GlobalOp>(getLoc(vd->getSourceRange()), name, type);
- // TODO(CIR): This code for processing initial values is a placeholder
- // until class ConstantEmitter is upstreamed and the code for processing
- // constant expressions is filled out. Only the most basic handling of
- // certain constant expressions is implemented for now.
- const VarDecl *initDecl;
- const Expr *initExpr = vd->getAnyInitializer(initDecl);
- mlir::Attribute initializer;
- if (initExpr) {
- if (APValue *value = initDecl->evaluateValue()) {
- ConstantEmitter emitter(*this);
- initializer = emitter.tryEmitPrivateForMemory(*value, astTy);
+
+ if (getLangOpts().OpenCL || getLangOpts().OpenMPIsTargetDevice) {
+ errorNYI(vd->getSourceRange(), "emit OpenCL/OpenMP global variable");
+ return;
+ }
+
+ mlir::Attribute init;
+ const VarDecl *initDecl;
+ const Expr *initExpr = vd->getAnyInitializer(initDecl);
+
+ std::optional<ConstantEmitter> emitter;
+
+ assert(!cir::MissingFeatures::cudaSupport());
+
+ if (vd->hasAttr<LoaderUninitializedAttr>()) {
+ errorNYI(vd->getSourceRange(), "loader uninitialized attribute");
+ return;
+ } else if (!initExpr) {
+ // This is a tentative definition; tentative definitions are
+ // implicitly initialized with { 0 }.
+ //
+ // Note that tentative definitions are only emitted at the end of
+ // a translation unit, so they should never have incomplete
+ // type. In addition, EmitTentativeDefinition makes sure that we
+ // never attempt to emit a tentative definition if a real one
+ // exists. A use may still exists, however, so we still may need
+ // to do a RAUW.
+ assert(!astTy->isIncompleteType() && "Unexpected incomplete type");
+ init = builder.getZeroInitAttr(convertType(vd->getType()));
+ } else {
+ emitter.emplace(*this);
+ auto initializer = emitter->tryEmitForInitializer(*initDecl);
+ if (!initializer) {
+ QualType qt = initExpr->getType();
+ if (vd->getType()->isReferenceType())
+ qt = vd->getType();
+
+ if (getLangOpts().CPlusPlus) {
+ if (initDecl->hasFlexibleArrayInit(astContext))
+ errorNYI(vd->getSourceRange(), "flexible array initializer");
+ init = builder.getZeroInitAttr(convertType(qt));
+ if (astContext.GetGVALinkageForVariable(vd) != GVA_AvailableExternally)
+ errorNYI(vd->getSourceRange(), "global constructor");
} else {
- errorNYI(initExpr->getSourceRange(), "non-constant initializer");
+ errorNYI(vd->getSourceRange(), "static initializer");
}
} else {
- initializer = builder.getZeroInitAttr(convertType(astTy));
+ init = initializer;
+ // We don't need an initializer, so remove the entry for the delayed
+ // initializer position (just in case this entry was delayed) if we
+ // also don't need to register a destructor.
+ if (vd->needsDestruction(astContext) == QualType::DK_cxx_destructor)
+ errorNYI(vd->getSourceRange(), "delayed destructor");
}
+ }
- varOp.setInitialValueAttr(initializer);
+ mlir::Type initType;
+ if (mlir::isa<mlir::SymbolRefAttr>(init)) {
+ errorNYI(vd->getSourceRange(), "global initializer is a symbol reference");
+ return;
+ } else {
+ assert(mlir::isa<mlir::TypedAttr>(init) && "This should have a type");
+ auto typedInitAttr = mlir::cast<mlir::TypedAttr>(init);
+ initType = typedInitAttr.getType();
+ }
+ assert(!mlir::isa<mlir::NoneType>(initType) && "Should have a type by now");
- // Set CIR's linkage type as appropriate.
- cir::GlobalLinkageKind linkage =
- getCIRLinkageVarDefinition(vd, /*IsConstant=*/false);
+ cir::GlobalOp gv =
+ getOrCreateCIRGlobal(vd, initType, ForDefinition_t(!isTentative));
+ // TODO(cir): Strip off pointer casts from Entry if we get them?
- // Set CIR linkage and DLL storage class.
- varOp.setLinkage(linkage);
+ if (!gv || gv.getSymType() != initType) {
+ errorNYI(vd->getSourceRange(), "global initializer with type mismatch");
+ return;
+ }
- if (linkage == cir::GlobalLinkageKind::CommonLinkage)
- errorNYI(initExpr->getSourceRange(), "common linkage");
+ assert(!cir::MissingFeatures::maybeHandleStaticInExternC());
- theModule.push_back(varOp);
- } else {
- errorNYI(vd->getSourceRange().getBegin(),
- "variable definition with a non-identifier for a name");
+ if (vd->hasAttr<AnnotateAttr>()) {
+ errorNYI(vd->getSourceRange(), "annotate global variable");
}
+
+ assert(!cir::MissingFeatures::opGlobalLinkage());
+
+ if (langOpts.CUDA) {
+ errorNYI(vd->getSourceRange(), "CUDA global variable");
+ }
+
+ // Set initializer and finalize emission
+ CIRGenModule::setInitializer(gv, init);
+ if (emitter)
+ emitter->finalize(gv);
+
+ // Set CIR's linkage type as appropriate.
+ cir::GlobalLinkageKind linkage =
+ getCIRLinkageVarDefinition(vd, /*IsConstant=*/false);
+
+ // Set CIR linkage and DLL storage class.
+ gv.setLinkage(linkage);
+
+ if (linkage == cir::GlobalLinkageKind::CommonLinkage)
+ errorNYI(initExpr->getSourceRange(), "common linkage");
}
void CIRGenModule::emitGlobalDefinition(clang::GlobalDecl gd,
@@ -684,6 +788,12 @@ void CIRGenModule::emitTopLevelDecl(Decl *decl) {
}
}
+void CIRGenModule::setInitializer(cir::GlobalOp &op, mlir::Attribute value) {
+ // Recompute visibility when updating initializer.
+ op.setInitialValueAttr(value);
+ assert(!cir::MissingFeatures::opGlobalSetVisitibility());
+}
+
cir::FuncOp CIRGenModule::getAddrOfFunction(clang::GlobalDecl gd,
mlir::Type funcType, bool forVTable,
bool dontDefer,
diff --git a/clang/lib/CIR/CodeGen/CIRGenModule.h b/clang/lib/CIR/CodeGen/CIRGenModule.h
index ea30903a97167..b67239fcff44b 100644
--- a/clang/lib/CIR/CodeGen/CIRGenModule.h
+++ b/clang/lib/CIR/CodeGen/CIRGenModule.h
@@ -122,6 +122,10 @@ class CIRGenModule : public CIRGenTypeCache {
cir::GlobalOp getOrCreateCIRGlobal(const VarDecl *d, mlir::Type ty,
ForDefinition_t isForDefinition);
+ static cir::GlobalOp createGlobalOp(CIRGenModule &cgm, mlir::Location loc,
+ llvm::StringRef name, mlir::Type t,
+ mlir::Operation *insertPoint = nullptr);
+
/// Return the mlir::Value for the address of the given global variable.
/// If Ty is non-null and if the global doesn't exist, then it will be created
/// with the specified type instead of whatever the normal requested type
@@ -179,6 +183,8 @@ class CIRGenModule : public CIRGenTypeCache {
llvm::StringRef getMangledName(clang::GlobalDecl gd);
+ static void setInitializer(cir::GlobalOp &op, mlir::Attribute value);
+
cir::FuncOp
getOrCreateCIRFunction(llvm::StringRef mangledName, mlir::Type funcType,
clang::GlobalDecl gd, bool forVTable,
|
erichkeane
reviewed
May 2, 2025
There was a problem hiding this comment.
Just 2 small comments, though I'd rather we get @bcardosolopes to take a lookhere.
| g = builder.create<cir::GlobalOp>(loc, name, t); | ||
| if (!curCGF) { | ||
| if (insertPoint) | ||
| cgm.getModule().insert(insertPoint, g); |
There was a problem hiding this comment.
It would be kind of neat here if a null-insertion point to 'insert' just got interpreted as the 'end' instead, to avoid us from having to do this.
There was a problem hiding this comment.
That would be nice, but I looked into it a bit and it won't work. This is just several layers of wrappers around an llvm::SymbolTableList, which is in turn a couple of wrappers around llvm::simple_list, which uses a sentinel value for its end iterator. If I try to pass null for insertPoint, it crashes in llvm::ilist_base<true, void>::insertBeforeImpl().
There was a problem hiding this comment.
I was thinking one of the wrappers could handle this along the way, but I guess it depends on how much we use this. It seems that I see ONE so far, but if it happens again we should look more seriously.
There was a problem hiding this comment.
I don't see any place else in the incubator where this happens. It seems this is the wrapper where it happens. Beyond this we're into general MLIR code, but a quick search didn't turn up any place there where this pattern occurred.
Generally, I don't think it matters where the entries get pushed in the list. The one place we are providing an insert point is when we're replacing a previously declared global. I'm not sure it matters there either honestly. but I guess it makes the output easier to read.
| CIRGenModule::createGlobalOp(*this, loc, mangledName, ty, | ||
| /*insertPoint=*/entry.getOperation()); | ||
|
|
||
| // errorNYI(d->getSourceRange(), "reference of undeclared global"); |
There was a problem hiding this comment.
Yeah, I'm not sure how I left that. I had put this aside in the middle of creating the PR, so I think maybe I wanted to take another look at what the incubator does after this. There is a bit more code here, but nothing we're ready for yet. It handles deferred declarations from inline class members, and handles (or has NYI markers for) attributes based the info from the VarDecl. I think I was pausing here to test whether I had enough for it to work as it is, and it appears I do.
erichkeane
approved these changes
May 2, 2025
AmrDeveloper
approved these changes
May 2, 2025
| @@ -46,6 +55,14 @@ class ConstantEmitter { | |||
| ConstantEmitter(const ConstantEmitter &other) = delete; | |||
| ConstantEmitter &operator=(const ConstantEmitter &other) = delete; | |||
|
|
|||
| ~ConstantEmitter(); | |||
|
|
|||
| /// Try to emit the initiaizer of the given declaration as an abstract | |||
There was a problem hiding this comment.
nit
Suggested change
| /// Try to emit the initiaizer of the given declaration as an abstract | |
| /// Try to emit the initializer of the given declaration as an abstract |
mmha
reviewed
May 2, 2025
| @@ -31,6 +31,15 @@ class ConstantEmitter { | |||
| private: | |||
| bool abstract = false; | |||
|
|
|||
| /// Whether non-abstract components of the emitter have been initialized. | |||
| bool initializedNonAbstract = false; | |||
There was a problem hiding this comment.
Is this just for sanity checking? Should this be wrapped in a #ifndef NDEBUG?
Same for finalized and failed.
There was a problem hiding this comment.
Yes, these are just for debugging. Wrapping them in NDEBUG makes sense.
| Expr *subExpr = e->getSubExpr(); | ||
|
|
||
| switch (e->getCastKind()) { | ||
| case CK_HLSLArrayRValue: |
There was a problem hiding this comment.
OGCG returns nullptr for the HLSL casts. The incubator has them as NYI.
There was a problem hiding this comment.
I can't see why we would need to do anything for them. Maybe the incubator placed them here at a time when OGCG was doing something with them? I believe Evaluate mentioned in the comment on line 136 is the AST's Evaluate function, which does seem to handle these cases.
| } | ||
|
|
||
| if (ile->getType()->isRecordType()) | ||
| cgm.errorNYI(ile->getBeginLoc(), "ConstExprEmitter: record ILE"); |
There was a problem hiding this comment.
I'd add an explicit return {}; here just from a defensive programming standpoint.
Should this also have its own MissingFeature assert?
There was a problem hiding this comment.
I've been using MissingFeature for places where we need to do something but either can't check for the conditions where we need to do it yet (because the check uses constructs that haven't been upstreamed yet) or we don't want to issue an error (as in the other two cases here). So, I don't think it would add anything here.
I'll add the explicit return.
| init = builder.getZeroInitAttr(convertType(vd->getType())); | ||
| } else { | ||
| emitter.emplace(*this); | ||
| auto initializer = emitter->tryEmitForInitializer(*initDecl); |
There was a problem hiding this comment.
Suggested change
| auto initializer = emitter->tryEmitForInitializer(*initDecl); | |
| mlir::Attribute initializer = emitter->tryEmitForInitializer(*initDecl); |
| // We don't need an initializer, so remove the entry for the delayed | ||
| // initializer position (just in case this entry was delayed) if we | ||
| // also don't need to register a destructor. | ||
| if (vd->needsDestruction(astContext) == QualType::DK_cxx_destructor) |
There was a problem hiding this comment.
What happened to isDefinitionAvailableExternally?
There was a problem hiding this comment.
I think it's only used in conjunction with constructors and destructors, so it isn't needed yet. I should probably drop an errorNYI or two for that.
There was a problem hiding this comment.
It looks like I missed a non-C++ case where it was used. I'll add it now.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/164/builds/9589 Here is the relevant piece of the build log for the reference |
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
When global variable support was initially upstreamed, we took some shortcuts and only implemented the minimum support for simple variables and constant initializers. This change refactors the code that creates global variables to introduce more of the complexities that are present in the incubator and the classic codegen. I can't really say this is NFC, because the code executed is very different and it will report different NYI diagnostics, but for the currently implemented cases, it results in the same output.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
When global variable support was initially upstreamed, we took some shortcuts and only implemented the minimum support for simple variables and constant initializers. This change refactors the code that creates global variables to introduce more of the complexities that are present in the incubator and the classic codegen. I can't really say this is NFC, because the code executed is very different and it will report different NYI diagnostics, but for the currently implemented cases, it results in the same output.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
When global variable support was initially upstreamed, we took some shortcuts and only implemented the minimum support for simple variables and constant initializers. This change refactors the code that creates global variables to introduce more of the complexities that are present in the incubator and the classic codegen. I can't really say this is NFC, because the code executed is very different and it will report different NYI diagnostics, but for the currently implemented cases, it results in the same output.
GeorgeARM
pushed a commit
to GeorgeARM/llvm-project
that referenced
this pull request
May 7, 2025
When global variable support was initially upstreamed, we took some shortcuts and only implemented the minimum support for simple variables and constant initializers. This change refactors the code that creates global variables to introduce more of the complexities that are present in the incubator and the classic codegen. I can't really say this is NFC, because the code executed is very different and it will report different NYI diagnostics, but for the currently implemented cases, it results in the same output.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When global variable support was initially upstreamed, we took some shortcuts and only implemented the minimum support for simple variables and constant initializers.
This change refactors the code that creates global variables to introduce more of the complexities that are present in the incubator and the classic codegen. I can't really say this is NFC, because the code executed is very different and it will report different NYI diagnostics, but for the currently implemented cases, it results in the same output.