[CIR] Generate the nsw flag correctly for unary ops #133815
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
A previous checkin used a workaround to generate the nsw flag where needed for unary ops. This change upstreams a subsequent change that was made in the incubator to generate the flag correctly.
andykaylor
requested review from
xlauko,
bcardosolopes,
erichkeane,
lanza,
AmrDeveloper,
dkolsen-pgi and
Lancern
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clangir Author: Andy Kaylor (andykaylor) ChangesA previous checkin used a workaround to generate the nsw flag where needed for unary ops. This change upstreams a subsequent change that was made in the incubator to generate the flag correctly. Full diff: https://github.com/llvm/llvm-project/pull/133815.diff 5 Files Affected:
diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td
index 455cc2b8b0277..33447e668cedd 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIROps.td
+++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td
@@ -702,10 +702,14 @@ def UnaryOp : CIR_Op<"unary", [Pure, SameOperandsAndResultType]> {
}];
let results = (outs CIR_AnyType:$result);
- let arguments = (ins Arg<UnaryOpKind, "unary op kind">:$kind, Arg<CIR_AnyType>:$input);
+ let arguments = (ins Arg<UnaryOpKind, "unary op kind">:$kind,
+ Arg<CIR_AnyType>:$input,
+ UnitAttr:$no_signed_wrap);
let assemblyFormat = [{
- `(` $kind `,` $input `)` `:` type($input) `,` type($result) attr-dict
+ `(` $kind `,` $input `)`
+ (`nsw` $no_signed_wrap^)?
+ `:` type($input) `,` type($result) attr-dict
}];
let hasVerifier = 1;
diff --git a/clang/include/clang/CIR/MissingFeatures.h b/clang/include/clang/CIR/MissingFeatures.h
index 3a102d90aba8f..23bf826d19a69 100644
--- a/clang/include/clang/CIR/MissingFeatures.h
+++ b/clang/include/clang/CIR/MissingFeatures.h
@@ -76,7 +76,6 @@ struct MissingFeatures {
static bool opScopeCleanupRegion() { return false; }
// Unary operator handling
- static bool opUnarySignedOverflow() { return false; }
static bool opUnaryPromotionType() { return false; }
// Clang early optimizations or things defered to LLVM lowering.
diff --git a/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp b/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp
index 2cf92dfbf3a5b..5ac1dc1052c2e 100644
--- a/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp
@@ -374,7 +374,7 @@ class ScalarExprEmitter : public StmtVisitor<ScalarExprEmitter, mlir::Value> {
cir::UnaryOpKind kind =
e->isIncrementOp() ? cir::UnaryOpKind::Inc : cir::UnaryOpKind::Dec;
// NOTE(CIR): clang calls CreateAdd but folds this to a unary op
- value = emitUnaryOp(e, kind, input);
+ value = emitUnaryOp(e, kind, input, /*nsw=*/false);
}
} else if (isa<PointerType>(type)) {
cgf.cgm.errorNYI(e->getSourceRange(), "Unary inc/dec pointer");
@@ -429,19 +429,17 @@ class ScalarExprEmitter : public StmtVisitor<ScalarExprEmitter, mlir::Value> {
mlir::Value emitIncDecConsiderOverflowBehavior(const UnaryOperator *e,
mlir::Value inVal,
bool isInc) {
- assert(!cir::MissingFeatures::opUnarySignedOverflow());
cir::UnaryOpKind kind =
e->isIncrementOp() ? cir::UnaryOpKind::Inc : cir::UnaryOpKind::Dec;
switch (cgf.getLangOpts().getSignedOverflowBehavior()) {
case LangOptions::SOB_Defined:
- return emitUnaryOp(e, kind, inVal);
+ return emitUnaryOp(e, kind, inVal, /*nsw=*/false);
case LangOptions::SOB_Undefined:
assert(!cir::MissingFeatures::sanitizers());
- return emitUnaryOp(e, kind, inVal);
- break;
+ return emitUnaryOp(e, kind, inVal, /*nsw=*/true);
case LangOptions::SOB_Trapping:
if (!e->canOverflow())
- return emitUnaryOp(e, kind, inVal);
+ return emitUnaryOp(e, kind, inVal, /*nsw=*/true);
cgf.cgm.errorNYI(e->getSourceRange(), "inc/def overflow SOB_Trapping");
return {};
}
@@ -473,18 +471,19 @@ class ScalarExprEmitter : public StmtVisitor<ScalarExprEmitter, mlir::Value> {
assert(!cir::MissingFeatures::opUnaryPromotionType());
mlir::Value operand = Visit(e->getSubExpr());
- assert(!cir::MissingFeatures::opUnarySignedOverflow());
+ bool nsw =
+ kind == cir::UnaryOpKind::Minus && e->getType()->isSignedIntegerType();
// NOTE: LLVM codegen will lower this directly to either a FNeg
// or a Sub instruction. In CIR this will be handled later in LowerToLLVM.
- return emitUnaryOp(e, kind, operand);
+ return emitUnaryOp(e, kind, operand, nsw);
}
mlir::Value emitUnaryOp(const UnaryOperator *e, cir::UnaryOpKind kind,
- mlir::Value input) {
+ mlir::Value input, bool nsw = false) {
return builder.create<cir::UnaryOp>(
cgf.getLoc(e->getSourceRange().getBegin()), input.getType(), kind,
- input);
+ input, nsw);
}
mlir::Value VisitUnaryNot(const UnaryOperator *e) {
diff --git a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
index b19be53947f99..48dc09d151dcf 100644
--- a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
+++ b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
@@ -860,14 +860,8 @@ mlir::LogicalResult CIRToLLVMUnaryOpLowering::matchAndRewrite(
// Integer unary operations: + - ~ ++ --
if (mlir::isa<cir::IntType>(elementType)) {
mlir::LLVM::IntegerOverflowFlags maybeNSW =
- mlir::LLVM::IntegerOverflowFlags::none;
- if (mlir::dyn_cast<cir::IntType>(elementType).isSigned()) {
- assert(!cir::MissingFeatures::opUnarySignedOverflow());
- // TODO: For now, assume signed overflow is undefined. We'll need to add
- // an attribute to the unary op to control this.
- maybeNSW = mlir::LLVM::IntegerOverflowFlags::nsw;
- }
-
+ op.getNoSignedWrap() ? mlir::LLVM::IntegerOverflowFlags::nsw
+ : mlir::LLVM::IntegerOverflowFlags::none;
switch (op.getKind()) {
case cir::UnaryOpKind::Inc: {
assert(!isVector && "++ not allowed on vector types");
diff --git a/clang/test/CIR/CodeGen/unary.cpp b/clang/test/CIR/CodeGen/unary.cpp
index 3e041e14ce177..ca47c1068e08d 100644
--- a/clang/test/CIR/CodeGen/unary.cpp
+++ b/clang/test/CIR/CodeGen/unary.cpp
@@ -83,7 +83,7 @@ int inc0() {
// CHECK: %[[ATMP:.*]] = cir.const #cir.int<1> : !s32i
// CHECK: cir.store %[[ATMP]], %[[A]] : !s32i
// CHECK: %[[INPUT:.*]] = cir.load %[[A]]
-// CHECK: %[[INCREMENTED:.*]] = cir.unary(inc, %[[INPUT]])
+// CHECK: %[[INCREMENTED:.*]] = cir.unary(inc, %[[INPUT]]) nsw
// CHECK: cir.store %[[INCREMENTED]], %[[A]]
// CHECK: %[[A_TO_OUTPUT:.*]] = cir.load %[[A]]
@@ -111,8 +111,8 @@ int dec0() {
// CHECK: %[[ATMP:.*]] = cir.const #cir.int<1> : !s32i
// CHECK: cir.store %[[ATMP]], %[[A]] : !s32i
// CHECK: %[[INPUT:.*]] = cir.load %[[A]]
-// CHECK: %[[INCREMENTED:.*]] = cir.unary(dec, %[[INPUT]])
-// CHECK: cir.store %[[INCREMENTED]], %[[A]]
+// CHECK: %[[DECREMENTED:.*]] = cir.unary(dec, %[[INPUT]]) nsw
+// CHECK: cir.store %[[DECREMENTED]], %[[A]]
// CHECK: %[[A_TO_OUTPUT:.*]] = cir.load %[[A]]
// LLVM: define i32 @dec0()
@@ -139,7 +139,7 @@ int inc1() {
// CHECK: %[[ATMP:.*]] = cir.const #cir.int<1> : !s32i
// CHECK: cir.store %[[ATMP]], %[[A]] : !s32i
// CHECK: %[[INPUT:.*]] = cir.load %[[A]]
-// CHECK: %[[INCREMENTED:.*]] = cir.unary(inc, %[[INPUT]])
+// CHECK: %[[INCREMENTED:.*]] = cir.unary(inc, %[[INPUT]]) nsw
// CHECK: cir.store %[[INCREMENTED]], %[[A]]
// CHECK: %[[A_TO_OUTPUT:.*]] = cir.load %[[A]]
@@ -167,8 +167,8 @@ int dec1() {
// CHECK: %[[ATMP:.*]] = cir.const #cir.int<1> : !s32i
// CHECK: cir.store %[[ATMP]], %[[A]] : !s32i
// CHECK: %[[INPUT:.*]] = cir.load %[[A]]
-// CHECK: %[[INCREMENTED:.*]] = cir.unary(dec, %[[INPUT]])
-// CHECK: cir.store %[[INCREMENTED]], %[[A]]
+// CHECK: %[[DECREMENTED:.*]] = cir.unary(dec, %[[INPUT]]) nsw
+// CHECK: cir.store %[[DECREMENTED]], %[[A]]
// CHECK: %[[A_TO_OUTPUT:.*]] = cir.load %[[A]]
// LLVM: define i32 @dec1()
@@ -197,7 +197,7 @@ int inc2() {
// CHECK: %[[ATMP:.*]] = cir.const #cir.int<1> : !s32i
// CHECK: cir.store %[[ATMP]], %[[A]] : !s32i
// CHECK: %[[ATOB:.*]] = cir.load %[[A]]
-// CHECK: %[[INCREMENTED:.*]] = cir.unary(inc, %[[ATOB]])
+// CHECK: %[[INCREMENTED:.*]] = cir.unary(inc, %[[ATOB]]) nsw
// CHECK: cir.store %[[INCREMENTED]], %[[A]]
// CHECK: cir.store %[[ATOB]], %[[B]]
// CHECK: %[[B_TO_OUTPUT:.*]] = cir.load %[[B]]
@@ -405,3 +405,22 @@ float fpPostInc2() {
// OGCG: store float %[[A_INC]], ptr %[[A]], align 4
// OGCG: store float %[[A_LOAD]], ptr %[[B]], align 4
// OGCG: %[[B_TO_OUTPUT:.*]] = load float, ptr %[[B]], align 4
+
+void chars(char c) {
+// CHECK: cir.func @chars
+
+ int c1 = +c;
+ // CHECK: %[[PROMO:.*]] = cir.cast(integral, %{{.+}} : !s8i), !s32i
+ // CHECK: cir.unary(plus, %[[PROMO]]) : !s32i, !s32i
+ int c2 = -c;
+ // CHECK: %[[PROMO:.*]] = cir.cast(integral, %{{.+}} : !s8i), !s32i
+ // CHECK: cir.unary(minus, %[[PROMO]]) nsw : !s32i, !s32i
+
+ // Chars can go through some integer promotion codegen paths even when not promoted.
+ // These should not have nsw attributes because the intermediate promotion makes the
+ // overflow defined behavior.
+ ++c; // CHECK: cir.unary(inc, %{{.+}}) : !s8i, !s8i
+ --c; // CHECK: cir.unary(dec, %{{.+}}) : !s8i, !s8i
+ c++; // CHECK: cir.unary(inc, %{{.+}}) : !s8i, !s8i
+ c--; // CHECK: cir.unary(dec, %{{.+}}) : !s8i, !s8i
+}
|
erichkeane
reviewed
Apr 1, 2025
| @@ -702,10 +702,14 @@ def UnaryOp : CIR_Op<"unary", [Pure, SameOperandsAndResultType]> { | |||
| }]; | |||
|
|
|||
| let results = (outs CIR_AnyType:$result); | |||
| let arguments = (ins Arg<UnaryOpKind, "unary op kind">:$kind, Arg<CIR_AnyType>:$input); | |||
| let arguments = (ins Arg<UnaryOpKind, "unary op kind">:$kind, | |||
There was a problem hiding this comment.
Should this be documented in the description since we're adding a new argument?
There was a problem hiding this comment.
Yeah. Good catch.
bcardosolopes
approved these changes
Apr 1, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/18/builds/13914 Here is the relevant piece of the build log for the reference |
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.
A previous checkin used a workaround to generate the nsw flag where needed for unary ops. This change upstreams a subsequent change that was made in the incubator to generate the flag correctly.