-
Notifications
You must be signed in to change notification settings - Fork 15.2k
feat: included support for is_constant builtin #166832
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
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clangir Author: Sathwik Kodamarthi (siddu0660) ChangesFull diff: https://github.com/llvm/llvm-project/pull/166832.diff 3 Files Affected:
diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td
index 2b361ed0982c6..9765684a1cd72 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIROps.td
+++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td
@@ -4052,6 +4052,44 @@ def CIR_ExpectOp : CIR_Op<"expect", [
}];
}
+//===----------------------------------------------------------------------===//
+// IsConstantOp
+//===----------------------------------------------------------------------===//
+
+def CIR_IsConstantOp : CIR_Op<"is_constant", [Pure]> {
+ let summary = "Check if a value is a compile-time constant";
+ let description = [{
+ The `cir.is_constant` operation checks whether its input value is a
+ compile-time constant. This operation models the `__builtin_constant_p`
+ builtin function.
+
+ The operation takes a single operand of any CIR type and returns a signed
+ 32-bit integer. The result is 1 if the operand is a compile-time constant,
+ and 0 otherwise.
+
+ If the value can be determined to be constant at compile time, this
+ operation may be folded to a constant value. Otherwise, it will be lowered
+ to the `llvm.is.constant` intrinsic.
+
+ Example:
+
+ ```mlir
+ %0 = cir.is_constant %expr : i32 -> !s32i
+ %1 = cir.is_constant %ptr : !cir.ptr<i32> -> !s32i
+ ```
+ }];
+
+ let arguments = (ins CIR_AnyType:$value);
+ let results = (outs CIR_SInt32:$result);
+
+ let assemblyFormat = [{
+ $value `:` type($value) `->` type($result) attr-dict
+ }];
+
+ let hasFolder = 1;
+ let hasLLVMLowering = false;
+}
+
//===----------------------------------------------------------------------===//
// PrefetchOp
//===----------------------------------------------------------------------===//
diff --git a/clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp b/clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp
index e35100ffe4b6b..8777ff16f068f 100644
--- a/clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp
@@ -199,6 +199,31 @@ RValue CIRGenFunction::emitBuiltinExpr(const GlobalDecl &gd, unsigned builtinID,
return RValue::get(
builder.createBitcast(allocaAddr, builder.getVoidPtrTy()));
}
+
+ case Builtin::BI__builtin_constant_p: {
+ auto loc = getLoc(e->getSourceRange());
+
+ Expr::EvalResult evalResult;
+
+ // Try to evaluate at compile time first
+ if (e->getArg(0)->EvaluateAsRValue(evalResult, getContext()) &&
+ !evalResult.hasSideEffects()) {
+ // Expression is a compile-time constant, return 1
+ llvm::APInt apInt(32, 1);
+ llvm::APSInt apSInt(apInt, /*isUnsigned=*/false);
+ return RValue::get(builder.getConstInt(loc, apSInt));
+ }
+
+ // Expression cannot be evaluated at compile time, emit runtime check
+ mlir::Value argValue = emitScalarExpr(e->getArg(0));
+ mlir::Type resultType = builder.getSInt32Ty();
+ auto isConstantOp = cir::IsConstantOp::create(builder, loc, resultType, argValue);
+ mlir::Value resultValue = isConstantOp.getResult();
+ mlir::Type exprTy = convertType(e->getType());
+ if (exprTy != resultValue.getType())
+ resultValue = builder.createIntCast(resultValue, exprTy);
+ return RValue::get(resultValue);
+ }
case Builtin::BIcos:
case Builtin::BIcosf:
diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
index 7ba03ce40140c..de23514cf5a79 100644
--- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
+++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
@@ -2117,6 +2117,25 @@ OpFoldResult cir::UnaryOp::fold(FoldAdaptor adaptor) {
return {};
}
+//===----------------------------------------------------------------------===//
+// IsConstantOp Definitions
+//===----------------------------------------------------------------------===//
+
+OpFoldResult cir::IsConstantOp::fold(FoldAdaptor adaptor) {
+ // If the input value is a constant attribute, return 1 (true)
+ mlir::Attribute value = adaptor.getValue();
+ if (value) {
+ // The value is a compile-time constant, so return 1
+ mlir::Type resultType = getResult().getType();
+ llvm::APInt apInt(32, 1);
+ llvm::APSInt apSInt(apInt, /*isUnsigned=*/false);
+ return cir::IntAttr::get(resultType, apSInt);
+ }
+
+ // If the input is not a constant, we cannot fold
+ return {};
+}
+
//===----------------------------------------------------------------------===//
// CopyOp Definitions
//===----------------------------------------------------------------------===//
|
|
Hi @siddu0660. Thanks for working on this! I appreciate that you were proactive in getting started on this. Unfortunately, that probably resulted in a bit of duplicated effort as @adams381 is also working on this, but I can see that you had probably started before I added the comment in #163889 explaining that. I'll review the PR in a minute, but my first feedback is that the title should begin with |
andykaylor
left a comment
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.
This needs a test and lowering to LLVM IR.
Typically, you'd create a PR from a branch in your fork other than main.
| // BinOpOverflow | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| def CIR_BinOpOverflow : CIR_Op<"binop.overflow", [Pure]> { |
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.
This isn't needed for this PR. Even if you were adding handlers for both sets of builtins, that would need to be done in separate PRs. We have a policy of keeping PRs focused on a single feature or bug fix.
|
|
||
| def CIR_IsConstantOp : CIR_Op<"is_constant", [Pure]> { | ||
| let summary = "Check if a value is a compile-time constant"; | ||
| let description = [{ |
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.
Great expansion of the incubator description. Thanks!
| } | ||
|
|
||
| //===----------------------------------------------------------------------===// | ||
| // IsConstantOp |
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.
How did you choose the placement of this? We generally try to keep things in the same relative order as they are in the incubator to help with comparisons.
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.
Thanks for mentioning the incubator ! I hadn’t come across that detail earlier, but I appreciate you pointing it out. I’ll go through it and make sure I’m aligned before moving forward.
| }]; | ||
|
|
||
| let arguments = (ins CIR_AnyType:$value); | ||
| let results = (outs CIR_SInt32:$result); |
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.
| let results = (outs CIR_SInt32:$result); | |
| let results = (outs CIR_BoolType:$result); |
Why did you change this from what is in the incubator?
| let results = (outs CIR_SInt32:$result); | ||
|
|
||
| let assemblyFormat = [{ | ||
| $value `:` type($value) `->` type($result) attr-dict |
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.
@xlauko Is this consistent with the style we're using now. I know you've been updating things recently, but I don't have a good sense for the expected style.
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.
@andykaylor pretty much yes. I am traying us to converge on style : argument types -> return types respectivelly : type if the operation can be determined by a single type. Bit of style guide is in: https://llvm.github.io/clangir/Dialect/cir-style-guide.html
| } | ||
|
|
||
| case Builtin::BI__builtin_constant_p: { | ||
| auto loc = getLoc(e->getSourceRange()); |
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.
| auto loc = getLoc(e->getSourceRange()); | |
| mlir::Location loc = getLoc(e->getSourceRange()); |
Upstream LLVM uses auto less liberally than the incubator.
https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
| Expr::EvalResult evalResult; | ||
|
|
||
| // Try to evaluate at compile time first | ||
| if (e->getArg(0)->EvaluateAsRValue(evalResult, getContext()) && |
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.
You've diverged from both the incubator and classic codegen here. That's OK if your changes improve the code while keeping the same semantics, but I don't think what you've done here meets those criteria. We need this to behave exactly like classic codegen. Please use the code from the incubator.
| }]; | ||
|
|
||
| let hasFolder = 1; | ||
| let hasLLVMLowering = false; |
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.
This should be lowered to LLVM IR. The lowering is simple enough to include in this PR.
|
|
||
| OpFoldResult cir::IsConstantOp::fold(FoldAdaptor adaptor) { | ||
| // If the input value is a constant attribute, return 1 (true) | ||
| mlir::Attribute value = adaptor.getValue(); |
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.
Have you tested this? It doesn't look right.
xlauko
left a comment
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.
Please add tests that test mlir roundtrip and also codegen to llvm vs OGCG.
| let results = (outs CIR_SInt32:$result); | ||
|
|
||
| let assemblyFormat = [{ | ||
| $value `:` type($value) `->` type($result) attr-dict |
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.
@andykaylor pretty much yes. I am traying us to converge on style : argument types -> return types respectivelly : type if the operation can be determined by a single type. Bit of style guide is in: https://llvm.github.io/clangir/Dialect/cir-style-guide.html
| let results = (outs CIR_BoolType:$result); | ||
|
|
||
| let assemblyFormat = [{ | ||
| `(` $value `:` type($value) `)` `:` type($result) attr-dict |
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.
| `(` $value `:` type($value) `)` `:` type($result) attr-dict | |
| `(` $value `:` qualified(type($value)) `)` `->` qualified(type($result)) attr-dict |
| `(` $value `:` type($value) `)` `:` type($result) attr-dict | ||
| }]; | ||
|
|
||
| // let hasFolder = 1; |
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.
Remove this or implement folder as well.
| }]; | ||
|
|
||
| // let hasFolder = 1; | ||
| // let hasLLVMLowering = false; |
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.
Why is this commented out?
| const Expr *Arg = e->getArg(0); | ||
| QualType ArgType = Arg->getType(); | ||
| // FIXME: The allowance for Obj-C pointers and block pointers is historical | ||
| // and likely a mistake. | ||
| if (!ArgType->isIntegralOrEnumerationType() && !ArgType->isFloatingType() && | ||
| !ArgType->isObjCObjectPointerType() && !ArgType->isBlockPointerType()) | ||
| // Per the GCC documentation, only numeric constants are recognized after | ||
| // inlining. | ||
| return RValue::get( | ||
| builder.getConstInt(getLoc(e->getSourceRange()), | ||
| mlir::cast<cir::IntType>(ResultType), 0)); | ||
|
|
||
| if (Arg->HasSideEffects(getContext())) | ||
| // The argument is unevaluated, so be conservative if it might have | ||
| // side-effects. | ||
| return RValue::get( | ||
| builder.getConstInt(getLoc(e->getSourceRange()), | ||
| mlir::cast<cir::IntType>(ResultType), 0)); | ||
|
|
||
| mlir::Value ArgValue = emitScalarExpr(Arg); | ||
| if (ArgType->isObjCObjectPointerType()) | ||
| // Convert Objective-C objects to id because we cannot distinguish between | ||
| // LLVM types for Obj-C classes as they are opaque. | ||
| ArgType = cgm.getASTContext().getObjCIdType(); | ||
| ArgValue = builder.createBitcast(ArgValue, convertType(ArgType)); | ||
|
|
||
| mlir::Value Result = cir::IsConstantOp::create( | ||
| builder, getLoc(e->getSourceRange()), ArgValue); | ||
| if (Result.getType() != ResultType) | ||
| Result = builder.createBoolToInt(Result, ResultType); | ||
| return RValue::get(Result); |
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.
use lower camelCase for variable names
| const Expr *Arg = e->getArg(0); | ||
| QualType ArgType = Arg->getType(); |
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.
| const Expr *Arg = e->getArg(0); | |
| QualType ArgType = Arg->getType(); | |
| const Expr *arg = e->getArg(0); | |
| QualType argType = arg->getType(); |
| // IsConstantOp Definitions | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| OpFoldResult cir::IsConstantOp::fold(FoldAdaptor adaptor) { |
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.
Folder is turned off in the operation, please set hasFolder = 1
| %0 = cir.is_constant %expr : i32 -> !s32i | ||
| %1 = cir.is_constant %ptr : !cir.ptr<i32> -> !s32i |
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.
| %0 = cir.is_constant %expr : i32 -> !s32i | |
| %1 = cir.is_constant %ptr : !cir.ptr<i32> -> !s32i | |
| %0 = cir.is_constant %expr : i32 -> !cir.bool | |
| %1 = cir.is_constant %ptr : !cir.ptr<i32> -> !cir.bool |
| The operation takes a single operand of any CIR type and returns a signed | ||
| 32-bit integer. The result is 1 if the operand is a compile-time constant, | ||
| and 0 otherwise. |
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.
This is not true, the expected result is bool
|
@siddu0660 @spamprx Any updates on this? I'd like to get handling for this builtin upstreamed soon. |
No description provided.