[CIR][NFC] Simplify BoolAttr builders #136366
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
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clangir Author: Henrich Lauko (xlauko) ChangesThis mirrors incubator changes from llvm/clangir#1572 Full diff: https://github.com/llvm/llvm-project/pull/136366.diff 6 Files Affected:
diff --git a/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h b/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h
index d2a241964f34f..ee8af62ede0da 100644
--- a/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h
+++ b/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h
@@ -57,6 +57,7 @@ class CIRBaseBuilderTy : public mlir::OpBuilder {
public:
CIRBaseBuilderTy(mlir::MLIRContext &mlirContext)
: mlir::OpBuilder(&mlirContext) {}
+ CIRBaseBuilderTy(mlir::OpBuilder &builder) : mlir::OpBuilder(builder) {}
mlir::Value getConstAPInt(mlir::Location loc, mlir::Type typ,
const llvm::APInt &val) {
@@ -98,13 +99,13 @@ class CIRBaseBuilderTy : public mlir::OpBuilder {
if (auto recordTy = mlir::dyn_cast<cir::RecordType>(ty))
return getZeroAttr(recordTy);
if (mlir::isa<cir::BoolType>(ty)) {
- return getCIRBoolAttr(false);
+ return getFalseAttr();
}
llvm_unreachable("Zero initializer for given type is NYI");
}
cir::ConstantOp getBool(bool state, mlir::Location loc) {
- return create<cir::ConstantOp>(loc, getBoolTy(), getCIRBoolAttr(state));
+ return create<cir::ConstantOp>(loc, getCIRBoolAttr(state));
}
cir::ConstantOp getFalse(mlir::Location loc) { return getBool(false, loc); }
cir::ConstantOp getTrue(mlir::Location loc) { return getBool(true, loc); }
@@ -120,9 +121,12 @@ class CIRBaseBuilderTy : public mlir::OpBuilder {
}
cir::BoolAttr getCIRBoolAttr(bool state) {
- return cir::BoolAttr::get(getContext(), getBoolTy(), state);
+ return cir::BoolAttr::get(getContext(), state);
}
+ cir::BoolAttr getTrueAttr() { return getCIRBoolAttr(true); }
+ cir::BoolAttr getFalseAttr() { return getCIRBoolAttr(false); }
+
mlir::Value createNot(mlir::Value value) {
return create<cir::UnaryOp>(value.getLoc(), value.getType(),
cir::UnaryOpKind::Not, value);
diff --git a/clang/include/clang/CIR/Dialect/IR/CIRAttrs.td b/clang/include/clang/CIR/Dialect/IR/CIRAttrs.td
index 3680ded4afafe..25ceded7e8a5b 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIRAttrs.td
+++ b/clang/include/clang/CIR/Dialect/IR/CIRAttrs.td
@@ -49,6 +49,12 @@ def CIR_BoolAttr : CIR_Attr<"Bool", "bool", [TypedAttrInterface]> {
"", "cir::BoolType">:$type,
"bool":$value);
+ let builders = [
+ AttrBuilder<(ins "bool":$value), [{
+ return $_get($_ctxt, cir::BoolType::get($_ctxt), value);
+ }]>,
+ ];
+
let assemblyFormat = [{
`<` $value `>`
}];
diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td
index 5ba4b33dc1a12..b526d077a910c 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIROps.td
+++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td
@@ -294,6 +294,12 @@ def ConstantOp : CIR_Op<"const",
// The constant operation returns a single value of CIR_AnyType.
let results = (outs CIR_AnyType:$res);
+ let builders = [
+ OpBuilder<(ins "cir::BoolAttr":$value), [{
+ build($_builder, $_state, value.getType(), value);
+ }]>
+ ];
+
let assemblyFormat = "attr-dict $value";
let hasVerifier = 1;
@@ -844,7 +850,7 @@ def UnaryOp : CIR_Op<"unary", [Pure, SameOperandsAndResultType]> {
let assemblyFormat = [{
`(` $kind `,` $input `)`
(`nsw` $no_signed_wrap^)?
- `:` type($input) `,` type($result) attr-dict
+ `:` type($input) `,` type($result) attr-dict
}];
let hasVerifier = 1;
diff --git a/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp b/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp
index 1bef1b976a4b5..f1561d1b26fc0 100644
--- a/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp
@@ -152,10 +152,7 @@ class ScalarExprEmitter : public StmtVisitor<ScalarExprEmitter, mlir::Value> {
}
mlir::Value VisitCXXBoolLiteralExpr(const CXXBoolLiteralExpr *e) {
- mlir::Type type = cgf.convertType(e->getType());
- return builder.create<cir::ConstantOp>(
- cgf.getLoc(e->getExprLoc()), type,
- builder.getCIRBoolAttr(e->getValue()));
+ return builder.getBool(e->getValue(), cgf.getLoc(e->getExprLoc()));
}
mlir::Value VisitCastExpr(CastExpr *e);
@@ -215,9 +212,7 @@ class ScalarExprEmitter : public StmtVisitor<ScalarExprEmitter, mlir::Value> {
if (llvm::isa<MemberPointerType>(srcType)) {
cgf.getCIRGenModule().errorNYI(loc, "member pointer to bool conversion");
- mlir::Type boolType = builder.getBoolTy();
- return builder.create<cir::ConstantOp>(loc, boolType,
- builder.getCIRBoolAttr(false));
+ return builder.getFalse(loc);
}
if (srcType->isIntegerType())
@@ -354,9 +349,7 @@ class ScalarExprEmitter : public StmtVisitor<ScalarExprEmitter, mlir::Value> {
// An interesting aspect of this is that increment is always true.
// Decrement does not have this property.
if (isInc && type->isBooleanType()) {
- value = builder.create<cir::ConstantOp>(cgf.getLoc(e->getExprLoc()),
- cgf.convertType(type),
- builder.getCIRBoolAttr(true));
+ value = builder.getTrue(cgf.getLoc(e->getExprLoc()));
} else if (type->isIntegerType()) {
QualType promotedType;
bool canPerformLossyDemotionCheck = false;
diff --git a/clang/lib/CIR/CodeGen/CIRGenStmt.cpp b/clang/lib/CIR/CodeGen/CIRGenStmt.cpp
index f503e2344f952..82ac53706b7f9 100644
--- a/clang/lib/CIR/CodeGen/CIRGenStmt.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenStmt.cpp
@@ -456,9 +456,7 @@ mlir::LogicalResult CIRGenFunction::emitForStmt(const ForStmt &s) {
// scalar type.
condVal = evaluateExprAsBool(s.getCond());
} else {
- cir::BoolType boolTy = cir::BoolType::get(b.getContext());
- condVal = b.create<cir::ConstantOp>(
- loc, boolTy, cir::BoolAttr::get(b.getContext(), boolTy, true));
+ condVal = b.create<cir::ConstantOp>(loc, builder.getTrueAttr());
}
builder.createCondition(condVal);
},
diff --git a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
index 8c4a67258df3f..774faa5b3b0be 100644
--- a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
+++ b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
@@ -692,9 +692,7 @@ mlir::LogicalResult CIRToLLVMConstantOpLowering::matchAndRewrite(
// during a pass as long as they don't live past the end of the pass.
attr = op.getValue();
} else if (mlir::isa<cir::BoolType>(op.getType())) {
- int value = (op.getValue() ==
- cir::BoolAttr::get(getContext(),
- cir::BoolType::get(getContext()), true));
+ int value = (op.getValue() == cir::BoolAttr::get(getContext(), true));
attr = rewriter.getIntegerAttr(typeConverter->convertType(op.getType()),
value);
} else if (mlir::isa<cir::IntType>(op.getType())) {
|
andykaylor
approved these changes
Apr 18, 2025
| cir::BoolType boolTy = cir::BoolType::get(b.getContext()); | ||
| condVal = b.create<cir::ConstantOp>( | ||
| loc, boolTy, cir::BoolAttr::get(b.getContext(), boolTy, true)); | ||
| condVal = b.create<cir::ConstantOp>(loc, builder.getTrueAttr()); |
There was a problem hiding this comment.
Suggested change
| condVal = b.create<cir::ConstantOp>(loc, builder.getTrueAttr()); | |
| condVal = b.getTrue(); |
Does this work?
There was a problem hiding this comment.
b is mlir::OpBuilder here which does not have this method.
It can be done as CIRBaseBuilderTy(b).getTrue().
| int value = (op.getValue() == | ||
| cir::BoolAttr::get(getContext(), | ||
| cir::BoolType::get(getContext()), true)); | ||
| int value = (op.getValue() == cir::BoolAttr::get(getContext(), true)); |
There was a problem hiding this comment.
Is there a reason we can't just use int value = mlir::cast<cir::BoolAttr>(op.getValue()).getValue() here?
There was a problem hiding this comment.
yeah, probably even nicer is
else if (auto ba = mlir::dyn_cast<cir::BoolAttr>(op.getValue())) {
attr = rewriter.getIntegerAttr(typeConverter->convertType(op.getType()), ba.getValue());
There was a problem hiding this comment.
This entire function might be more nicer in terms of cast to attributes directly, not dispatch based on types.
Also nobody guarantees, that if attribute has cir::BoolType it is actually cir::BoolAttr,
so the previous code might have returned false even in cases that attribute hold "true" value.
I will look into it in next PR.
There was a problem hiding this comment.
Fixed as suggested for now. Also in incubator: llvm/clangir#1573
xlauko
force-pushed
the
users/xlauko/cir-bool-attr-builders
branch
from
510777f to
9b33421
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/16358 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
This mirrors incubator changes from llvm/clangir#1572
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
This mirrors incubator changes from llvm/clangir#1572
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
This mirrors incubator changes from llvm/clangir#1572
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.
This mirrors incubator changes from llvm/clangir#1572