[CIR][NFC] Upstream LValueBaseInfo handling #134928
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
andykaylor
requested review from
bcardosolopes,
erichkeane,
lanza,
AmrDeveloper and
dkolsen-pgi
llvmbot
added
clang
|
@llvm/pr-subscribers-clangir @llvm/pr-subscribers-clang Author: Andy Kaylor (andykaylor) ChangesPrevious implementations that used the cir::LValue class omitted hanling of the LValueBaseInfo class, which tracks information about the basis for the LValue's alignment. As more code was upstreamed from the incubator, we were accumulating technical debt by adding more places where this wasn't handled correctly. This change puts the interfaces in place to track this information. The information being tracked isn't used yet, so no functional change is intended. The tracking is being added now because it will become more difficult to add it as more features are implemented. Full diff: https://github.com/llvm/llvm-project/pull/134928.diff 8 Files Affected:
diff --git a/clang/include/clang/CIR/MissingFeatures.h b/clang/include/clang/CIR/MissingFeatures.h
index ed952620e723e..189e31ce6ffd2 100644
--- a/clang/include/clang/CIR/MissingFeatures.h
+++ b/clang/include/clang/CIR/MissingFeatures.h
@@ -107,9 +107,9 @@ struct MissingFeatures {
static bool cgFPOptionsRAII() { return false; }
static bool metaDataNode() { return false; }
static bool fastMathFlags() { return false; }
- static bool lvalueBaseInfo() { return false; }
static bool alignCXXRecordDecl() { return false; }
static bool setNonGC() { return false; }
+ static bool objCGC() { return false; }
// Missing types
static bool dataMemberType() { return false; }
diff --git a/clang/lib/CIR/CodeGen/CIRGenDecl.cpp b/clang/lib/CIR/CodeGen/CIRGenDecl.cpp
index d0eb648683e8c..b8e72f299acb6 100644
--- a/clang/lib/CIR/CodeGen/CIRGenDecl.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenDecl.cpp
@@ -146,7 +146,7 @@ void CIRGenFunction::emitAutoVarInit(
// its removal/optimization to the CIR lowering.
if (!constant || isa<CXXTemporaryObjectExpr>(init)) {
initializeWhatIsTechnicallyUninitialized(addr);
- LValue lv = LValue::makeAddr(addr, type);
+ LValue lv = makeAddrLValue(addr, type, AlignmentSource::Decl);
emitExprAsInit(init, &d, lv);
// In case lv has uses it means we indeed initialized something
// out of it while trying to build the expression, mark it as such.
@@ -165,7 +165,7 @@ void CIRGenFunction::emitAutoVarInit(
assert(typedConstant && "expected typed attribute");
if (!emission.IsConstantAggregate) {
// For simple scalar/complex initialization, store the value directly.
- LValue lv = LValue::makeAddr(addr, type);
+ LValue lv = makeAddrLValue(addr, type);
assert(init && "expected initializer");
mlir::Location initLoc = getLoc(init->getSourceRange());
// lv.setNonGC(true);
diff --git a/clang/lib/CIR/CodeGen/CIRGenExpr.cpp b/clang/lib/CIR/CodeGen/CIRGenExpr.cpp
index fe3f2f5de249c..b8e2c73bf6080 100644
--- a/clang/lib/CIR/CodeGen/CIRGenExpr.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenExpr.cpp
@@ -27,7 +27,8 @@ using namespace cir;
/// Given an expression of pointer type, try to
/// derive a more accurate bound on the alignment of the pointer.
-Address CIRGenFunction::emitPointerWithAlignment(const Expr *expr) {
+Address CIRGenFunction::emitPointerWithAlignment(const Expr *expr,
+ LValueBaseInfo *baseInfo) {
// We allow this with ObjC object pointers because of fragile ABIs.
assert(expr->getType()->isPointerType() ||
expr->getType()->isObjCObjectPointerType());
@@ -163,7 +164,8 @@ Address CIRGenFunction::emitPointerWithAlignment(const Expr *expr) {
// Otherwise, use the alignment of the type.
return makeNaturalAddressForPointer(
- emitScalarExpr(expr), expr->getType()->getPointeeType(), CharUnits());
+ emitScalarExpr(expr), expr->getType()->getPointeeType(), CharUnits(),
+ /*forPointeeType=*/true, baseInfo);
}
void CIRGenFunction::emitStoreThroughLValue(RValue src, LValue dst,
@@ -299,7 +301,7 @@ LValue CIRGenFunction::emitDeclRefLValue(const DeclRefExpr *e) {
cgm.errorNYI(vd->getSourceRange(), "emitDeclRefLValue: static local");
}
- return LValue::makeAddr(addr, ty);
+ return makeAddrLValue(addr, ty, AlignmentSource::Type);
}
cgm.errorNYI(e->getSourceRange(), "emitDeclRefLValue: unhandled decl type");
@@ -337,9 +339,9 @@ LValue CIRGenFunction::emitUnaryOpLValue(const UnaryOperator *e) {
QualType t = e->getSubExpr()->getType()->getPointeeType();
assert(!t.isNull() && "CodeGenFunction::EmitUnaryOpLValue: Illegal type");
- assert(!cir::MissingFeatures::lvalueBaseInfo());
assert(!cir::MissingFeatures::opTBAA());
- Address addr = emitPointerWithAlignment(e->getSubExpr());
+ LValueBaseInfo baseInfo;
+ Address addr = emitPointerWithAlignment(e->getSubExpr(), &baseInfo);
// Tag 'load' with deref attribute.
// FIXME: This misses some derefence cases and has problematic interactions
@@ -349,7 +351,7 @@ LValue CIRGenFunction::emitUnaryOpLValue(const UnaryOperator *e) {
loadOp.setIsDerefAttr(mlir::UnitAttr::get(&getMLIRContext()));
}
- LValue lv = LValue::makeAddr(addr, t);
+ LValue lv = makeAddrLValue(addr, t, baseInfo);
assert(!cir::MissingFeatures::addressSpace());
assert(!cir::MissingFeatures::setNonGC());
return lv;
diff --git a/clang/lib/CIR/CodeGen/CIRGenExprAggregate.cpp b/clang/lib/CIR/CodeGen/CIRGenExprAggregate.cpp
index 36da63d5f7d76..368a6cb27c0fd 100644
--- a/clang/lib/CIR/CodeGen/CIRGenExprAggregate.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenExprAggregate.cpp
@@ -130,7 +130,7 @@ void AggExprEmitter::emitArrayInit(Address destPtr, cir::ArrayType arrayTy,
}
const Address address = Address(element, cirElementType, elementAlign);
- const LValue elementLV = LValue::makeAddr(address, elementType);
+ const LValue elementLV = cgf.makeAddrLValue(address, elementType);
emitInitializationToLValue(args[i], elementLV);
}
@@ -157,7 +157,7 @@ void AggExprEmitter::emitArrayInit(Address destPtr, cir::ArrayType arrayTy,
const Address tmpAddr = cgf.createTempAlloca(
cirElementPtrType, cgf.getPointerAlign(), loc, "arrayinit.temp",
/*insertIntoFnEntryBlock=*/false);
- LValue tmpLV = LValue::makeAddr(tmpAddr, elementPtrType);
+ LValue tmpLV = cgf.makeAddrLValue(tmpAddr, elementPtrType);
cgf.emitStoreThroughLValue(RValue::get(element), tmpLV);
// TODO(CIR): Replace this part later with cir::DoWhileOp
@@ -166,7 +166,7 @@ void AggExprEmitter::emitArrayInit(Address destPtr, cir::ArrayType arrayTy,
builder.createLoad(loc, tmpAddr.getPointer());
// Emit the actual filler expression.
- const LValue elementLV = LValue::makeAddr(
+ const LValue elementLV = cgf.makeAddrLValue(
Address(currentElement, cirElementType, elementAlign), elementType);
if (arrayFiller)
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.h b/clang/lib/CIR/CodeGen/CIRGenFunction.h
index dde665a75ae57..f0c146315a673 100644
--- a/clang/lib/CIR/CodeGen/CIRGenFunction.h
+++ b/clang/lib/CIR/CodeGen/CIRGenFunction.h
@@ -227,12 +227,23 @@ class CIRGenFunction : public CIRGenTypeCache {
/// been signed, and the returned Address will have the pointer authentication
/// information needed to authenticate the signed pointer.
Address makeNaturalAddressForPointer(mlir::Value ptr, QualType t,
- CharUnits alignment) {
+ CharUnits alignment,
+ bool forPointeeType = false,
+ LValueBaseInfo *baseInfo = nullptr) {
if (alignment.isZero())
- alignment = cgm.getNaturalTypeAlignment(t);
+ alignment = cgm.getNaturalTypeAlignment(t, baseInfo);
return Address(ptr, convertTypeForMem(t), alignment);
}
+ LValue makeAddrLValue(Address addr, QualType ty,
+ AlignmentSource source = AlignmentSource::Type) {
+ return makeAddrLValue(addr, ty, LValueBaseInfo(source));
+ }
+
+ LValue makeAddrLValue(Address addr, QualType ty, LValueBaseInfo baseInfo) {
+ return LValue::makeAddr(addr, ty, baseInfo);
+ }
+
cir::FuncOp generateCode(clang::GlobalDecl gd, cir::FuncOp fn,
cir::FuncType funcType);
@@ -489,7 +500,8 @@ class CIRGenFunction : public CIRGenTypeCache {
/// into the address of a local variable. In such a case, it's quite
/// reasonable to just ignore the returned alignment when it isn't from an
/// explicit source.
- Address emitPointerWithAlignment(const clang::Expr *expr);
+ Address emitPointerWithAlignment(const clang::Expr *expr,
+ LValueBaseInfo *baseInfo);
mlir::LogicalResult emitReturnStmt(const clang::ReturnStmt &s);
diff --git a/clang/lib/CIR/CodeGen/CIRGenModule.cpp b/clang/lib/CIR/CodeGen/CIRGenModule.cpp
index 78d995b4d9648..d2259a9c41d22 100644
--- a/clang/lib/CIR/CodeGen/CIRGenModule.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenModule.cpp
@@ -74,18 +74,20 @@ CIRGenModule::CIRGenModule(mlir::MLIRContext &mlirContext,
builder.getStringAttr(getTriple().str()));
}
-CharUnits CIRGenModule::getNaturalTypeAlignment(QualType t) {
+CharUnits CIRGenModule::getNaturalTypeAlignment(QualType t,
+ LValueBaseInfo *baseInfo) {
assert(!cir::MissingFeatures::opTBAA());
- // FIXME: This duplicates logic in ASTContext::getTypeAlignIfKnown. But
- // that doesn't return the information we need to compute BaseInfo.
+ // FIXME: This duplicates logic in ASTContext::getTypeAlignIfKnown, but
+ // that doesn't return the information we need to compute baseInfo.
// Honor alignment typedef attributes even on incomplete types.
// We also honor them straight for C++ class types, even as pointees;
// there's an expressivity gap here.
if (const auto *tt = t->getAs<TypedefType>()) {
if (unsigned align = tt->getDecl()->getMaxAlignment()) {
- assert(!cir::MissingFeatures::lvalueBaseInfo());
+ if (baseInfo)
+ *baseInfo = LValueBaseInfo(AlignmentSource::AttributedType);
return astContext.toCharUnitsFromBits(align);
}
}
@@ -99,13 +101,15 @@ CharUnits CIRGenModule::getNaturalTypeAlignment(QualType t) {
// ASTContext::getTypeAlignIfKnown, but nothing uses the alignment if the
// type is incomplete, so it's impossible to test. We could try to reuse
// getTypeAlignIfKnown, but that doesn't return the information we need
- // to set BaseInfo. So just ignore the possibility that the alignment is
+ // to set baseInfo. So just ignore the possibility that the alignment is
// greater than one.
- assert(!cir::MissingFeatures::lvalueBaseInfo());
+ if (baseInfo)
+ *baseInfo = LValueBaseInfo(AlignmentSource::Type);
return CharUnits::One();
}
- assert(!cir::MissingFeatures::lvalueBaseInfo());
+ if (baseInfo)
+ *baseInfo = LValueBaseInfo(AlignmentSource::Type);
CharUnits alignment;
if (t.getQualifiers().hasUnaligned()) {
diff --git a/clang/lib/CIR/CodeGen/CIRGenModule.h b/clang/lib/CIR/CodeGen/CIRGenModule.h
index 66e9faa5c48e5..fe39244fc7834 100644
--- a/clang/lib/CIR/CodeGen/CIRGenModule.h
+++ b/clang/lib/CIR/CodeGen/CIRGenModule.h
@@ -16,6 +16,7 @@
#include "CIRGenBuilder.h"
#include "CIRGenTypeCache.h"
#include "CIRGenTypes.h"
+#include "CIRGenValue.h"
#include "clang/AST/CharUnits.h"
#include "clang/CIR/Dialect/IR/CIRDialect.h"
@@ -91,7 +92,8 @@ class CIRGenModule : public CIRGenTypeCache {
/// FIXME: this could likely be a common helper and not necessarily related
/// with codegen.
- clang::CharUnits getNaturalTypeAlignment(clang::QualType t);
+ clang::CharUnits getNaturalTypeAlignment(clang::QualType t,
+ LValueBaseInfo *baseInfo);
void emitTopLevelDecl(clang::Decl *decl);
diff --git a/clang/lib/CIR/CodeGen/CIRGenValue.h b/clang/lib/CIR/CodeGen/CIRGenValue.h
index 68aecc6ee4a10..1b702daae4b4c 100644
--- a/clang/lib/CIR/CodeGen/CIRGenValue.h
+++ b/clang/lib/CIR/CodeGen/CIRGenValue.h
@@ -75,6 +75,28 @@ enum class AlignmentSource {
Type
};
+/// Given that the base address has the given alignment source, what's
+/// our confidence in the alignment of the field?
+static inline AlignmentSource getFieldAlignmentSource(AlignmentSource source) {
+ // For now, we don't distinguish fields of opaque pointers from
+ // top-level declarations, but maybe we should.
+ return AlignmentSource::Decl;
+}
+
+class LValueBaseInfo {
+ AlignmentSource alignSource;
+
+public:
+ explicit LValueBaseInfo(AlignmentSource source = AlignmentSource::Type)
+ : alignSource(source) {}
+ AlignmentSource getAlignmentSource() const { return alignSource; }
+ void setAlignmentSource(AlignmentSource source) { alignSource = source; }
+
+ void mergeForCast(const LValueBaseInfo &info) {
+ setAlignmentSource(info.getAlignmentSource());
+ }
+};
+
class LValue {
enum {
Simple, // This is a normal l-value, use getAddress().
@@ -87,13 +109,26 @@ class LValue {
clang::QualType type;
clang::Qualifiers quals;
+ // The alignment to use when accessing this lvalue. (For vector elements,
+ // this is the alignment of the whole vector)
+ unsigned alignment;
mlir::Value v;
mlir::Type elementType;
+ LValueBaseInfo baseInfo;
- void initialize(clang::QualType type, clang::Qualifiers quals) {
- assert(!cir::MissingFeatures::lvalueBaseInfo());
+ void initialize(clang::QualType type, clang::Qualifiers quals,
+ clang::CharUnits alignment, LValueBaseInfo baseInfo) {
+ assert((!alignment.isZero() || type->isIncompleteType()) &&
+ "initializing l-value with zero alignment!");
this->type = type;
this->quals = quals;
+ const unsigned maxAlign = 1U << 31;
+ this->alignment = alignment.getQuantity() <= maxAlign
+ ? alignment.getQuantity()
+ : maxAlign;
+ assert(this->alignment == alignment.getQuantity() &&
+ "Alignment exceeds allowed max!");
+ this->baseInfo = baseInfo;
}
public:
@@ -108,9 +143,9 @@ class LValue {
mlir::Value getPointer() const { return v; }
clang::CharUnits getAlignment() const {
- // TODO: Handle alignment
- return clang::CharUnits::One();
+ return clang::CharUnits::fromQuantity(alignment);
}
+ void setAlignment(clang::CharUnits a) { alignment = a.getQuantity(); }
Address getAddress() const {
return Address(getPointer(), elementType, getAlignment());
@@ -118,13 +153,17 @@ class LValue {
const clang::Qualifiers &getQuals() const { return quals; }
- static LValue makeAddr(Address address, clang::QualType t) {
+ static LValue makeAddr(Address address, clang::QualType t,
+ LValueBaseInfo baseInfo) {
+ // Classic codegen sets the objc gc qualifier here. That requires an
+ // ASTContext, which is passed in from CIRGenFunction::makeAddrLValue.
+ assert(!cir::MissingFeatures::objCGC());
+
LValue r;
r.lvType = Simple;
r.v = address.getPointer();
r.elementType = address.getElementType();
- r.initialize(t, t.getQualifiers());
- assert(!cir::MissingFeatures::lvalueBaseInfo());
+ r.initialize(t, t.getQualifiers(), address.getAlignment(), baseInfo);
return r;
}
};
|
erichkeane
reviewed
Apr 8, 2025
AmrDeveloper
reviewed
Apr 8, 2025
| @@ -27,7 +27,8 @@ using namespace cir; | |||
|
|
|||
| /// Given an expression of pointer type, try to | |||
| /// derive a more accurate bound on the alignment of the pointer. | |||
| Address CIRGenFunction::emitPointerWithAlignment(const Expr *expr) { | |||
| Address CIRGenFunction::emitPointerWithAlignment(const Expr *expr, | |||
| LValueBaseInfo *baseInfo) { | |||
There was a problem hiding this comment.
We have a new warning from the previous PR related to unused variable, should we quickly handle it here?
if (auto const *ece = dyn_cast<ExplicitCastExpr>(ce)) { <----- ece
cgm.errorNYI(expr->getSourceRange(),
"emitPointerWithAlignment: explicit cast");
return Address::invalid();
}
There was a problem hiding this comment.
No. It's best to keep changes like that separate to avoid mingling unrelated changes in the commit history. We wouldn't want a warning to be reintroduced if the commit for this PR were reverted, for example. Thanks for brining this to my attention though, I'll take care of it.
|
@mmha Still having to add you as a reviewer by comment |
Previous implementations that used the cir::LValue class omitted hanling of the LValueBaseInfo class, which tracks information about the basis for the LValue's alignment. As more code was upstreamed from the incubator, we were accumulating technical debt by adding more places where this wasn't handled correctly. This change puts the interfaces in place to track this information. The information being tracked isn't used yet, so no functional change is intended. The tracking is being added now because it will become more difficult to add it as more features are implemented.
andykaylor
force-pushed
the
cir-aligninfo
branch
from
e87922e to
e9b227d
Compare
AllinLeeYL
pushed a commit
to AllinLeeYL/llvm-project
that referenced
this pull request
Apr 10, 2025
Previous implementations that used the cir::LValue class omitted hanling of the LValueBaseInfo class, which tracks information about the basis for the LValue's alignment. As more code was upstreamed from the incubator, we were accumulating technical debt by adding more places where this wasn't handled correctly. This change puts the interfaces in place to track this information. The information being tracked isn't used yet, so no functional change is intended. The tracking is being added now because it will become more difficult to add it as more features are implemented.
var-const
pushed a commit
to ldionne/llvm-project
that referenced
this pull request
Apr 17, 2025
Previous implementations that used the cir::LValue class omitted hanling of the LValueBaseInfo class, which tracks information about the basis for the LValue's alignment. As more code was upstreamed from the incubator, we were accumulating technical debt by adding more places where this wasn't handled correctly. This change puts the interfaces in place to track this information. The information being tracked isn't used yet, so no functional change is intended. The tracking is being added now because it will become more difficult to add it as more features are implemented.
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.
Previous implementations that used the cir::LValue class omitted hanling of the LValueBaseInfo class, which tracks information about the basis for the LValue's alignment. As more code was upstreamed from the incubator, we were accumulating technical debt by adding more places where this wasn't handled correctly. This change puts the interfaces in place to track this information.
The information being tracked isn't used yet, so no functional change is intended. The tracking is being added now because it will become more difficult to add it as more features are implemented.