[CIR] Fix assignment ignore in ScalarExprEmitter #166118
Open
+123 −6
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
We are missing a couple of cases were we are not supposed to ignore assignment results but did so, which results in compiler crashes. Fix that.
mmha requested review from andykaylor, bcardosolopes, lanza and xlauko as code owners 
llvmbot added clang 
| @llvm/pr-subscribers-clang Author: Morris Hafner (mmha) ChangesWe are missing a couple of cases were we are not supposed to ignore assignment results but did so, which results in compiler crashes. Fix that. Full diff: https://github.com/llvm/llvm-project/pull/166118.diff 2 Files Affected:
diff --git a/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp b/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp index 119314fe27dce..4156d8cb873da 100644 --- a/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp +++ b/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp @@ -78,7 +78,7 @@ struct BinOpInfo { class ScalarExprEmitter : public StmtVisitor<ScalarExprEmitter, mlir::Value> { CIRGenFunction &cgf; CIRGenBuilderTy &builder; - bool ignoreResultAssign; + bool ignoreResultAssign = false; public: ScalarExprEmitter(CIRGenFunction &cgf, CIRGenBuilderTy &builder) @@ -221,6 +221,8 @@ class ScalarExprEmitter : public StmtVisitor<ScalarExprEmitter, mlir::Value> { } mlir::Value VisitArraySubscriptExpr(ArraySubscriptExpr *e) { + ignoreResultAssign = false; + if (e->getBase()->getType()->isVectorType()) { assert(!cir::MissingFeatures::scalableVectors()); @@ -839,6 +841,7 @@ class ScalarExprEmitter : public StmtVisitor<ScalarExprEmitter, mlir::Value> { BinOpInfo emitBinOps(const BinaryOperator *e, QualType promotionType = QualType()) { + ignoreResultAssign = false; BinOpInfo result; result.lhs = cgf.emitPromotedScalarExpr(e->getLHS(), promotionType); result.rhs = cgf.emitPromotedScalarExpr(e->getRHS(), promotionType); @@ -924,6 +927,7 @@ class ScalarExprEmitter : public StmtVisitor<ScalarExprEmitter, mlir::Value> { #undef HANDLEBINOP mlir::Value emitCmp(const BinaryOperator *e) { + ignoreResultAssign = false; const mlir::Location loc = cgf.getLoc(e->getExprLoc()); mlir::Value result; QualType lhsTy = e->getLHS()->getType(); @@ -2054,6 +2058,11 @@ mlir::Value ScalarExprEmitter::VisitMemberExpr(MemberExpr *e) { mlir::Value ScalarExprEmitter::VisitInitListExpr(InitListExpr *e) { const unsigned numInitElements = e->getNumInits(); + [[maybe_unused]] const bool ignore = std::exchange(ignoreResultAssign, false); + assert((ignore == false || + (numInitElements == 0 && e->getType()->isVoidType())) && + "init list ignored"); + if (e->hadArrayRangeDesignator()) { cgf.cgm.errorNYI(e->getSourceRange(), "ArrayRangeDesignator"); return {}; diff --git a/clang/test/CIR/CodeGen/binassign.c b/clang/test/CIR/CodeGen/binassign.c index 44c54b4a2969a..966c0e921f346 100644 --- a/clang/test/CIR/CodeGen/binassign.c +++ b/clang/test/CIR/CodeGen/binassign.c @@ -100,3 +100,67 @@ void binary_assign_struct() { // OGCG: call void @llvm.memcpy.p0.p0.i64(ptr align 4 %[[LS_PTR]], ptr align 4 @gs, i64 8, i1 false) // OGCG: call void @llvm.memcpy.p0.p0.i64(ptr align 4 %[[LSV_PTR]], ptr align 4 @gsv, i64 8, i1 true) // OGCG: ret void + +int ignore_result_assign() { + int *p, *q = 0; + if(p = q) + return 1; + return 0; +} + +// CIR-LABEL: cir.func{{.*}} @ignore_result_assign() -> !s32i +// CIR: %[[RETVAL:.*]] = cir.alloca !s32i, !cir.ptr<!s32i>, ["__retval"] +// CIR: %[[P:.*]] = cir.alloca !cir.ptr<!s32i>, !cir.ptr<!cir.ptr<!s32i>>, ["p"] +// CIR: %[[Q:.*]] = cir.alloca !cir.ptr<!s32i>, !cir.ptr<!cir.ptr<!s32i>>, ["q", init] +// CIR: %[[NULL:.*]] = cir.const #cir.ptr<null> : !cir.ptr<!s32i> +// CIR: cir.store{{.*}} %[[NULL]], %[[Q]] : !cir.ptr<!s32i>, !cir.ptr<!cir.ptr<!s32i>> +// CIR: %[[Q_VAL:.*]] = cir.load{{.*}} %[[Q]] : !cir.ptr<!cir.ptr<!s32i>>, !cir.ptr<!s32i> +// CIR: cir.store{{.*}} %[[Q_VAL]], %[[P]] : !cir.ptr<!s32i>, !cir.ptr<!cir.ptr<!s32i>> +// CIR: %[[COND:.*]] = cir.cast ptr_to_bool %[[Q_VAL]] : !cir.ptr<!s32i> -> !cir.bool +// CIR: cir.if %[[COND]] { +// CIR: %[[ONE:.*]] = cir.const #cir.int<1> : !s32i +// CIR: cir.store %[[ONE]], %[[RETVAL]] : !s32i, !cir.ptr<!s32i> +// CIR: %{{.*}} = cir.load %[[RETVAL]] : !cir.ptr<!s32i>, !s32i +// CIR: cir.return +// CIR: } +// CIR: %[[ZERO:.*]] = cir.const #cir.int<0> : !s32i +// CIR: cir.store %[[ZERO]], %[[RETVAL]] : !s32i, !cir.ptr<!s32i> +// CIR: %{{.*}} = cir.load %[[RETVAL]] : !cir.ptr<!s32i>, !s32i +// CIR: cir.return + +// LLVM-LABEL: define {{.*}}i32 @ignore_result_assign() +// LLVM: %[[RETVAL_PTR:.*]] = alloca i32 +// LLVM: %[[P_PTR:.*]] = alloca ptr +// LLVM: %[[Q_PTR:.*]] = alloca ptr +// LLVM: store ptr null, ptr %[[Q_PTR]] +// LLVM: %[[Q_VAL:.*]] = load ptr, ptr %[[Q_PTR]] +// LLVM: store ptr %[[Q_VAL]], ptr %[[P_PTR]] +// LLVM: %[[CMP:.*]] = icmp ne ptr %[[Q_VAL]], null +// LLVM: br i1 %[[CMP]], label %[[THEN:.*]], label %[[ELSE:.*]] +// LLVM: [[THEN]]: +// LLVM: store i32 1, ptr %[[RETVAL_PTR]] +// LLVM: %{{.*}} = load i32, ptr %[[RETVAL_PTR]] +// LLVM: ret i32 +// LLVM: [[ELSE]]: +// LLVM: store i32 0, ptr %[[RETVAL_PTR]] +// LLVM: %{{.*}} = load i32, ptr %[[RETVAL_PTR]] +// LLVM: ret i32 + +// OGCG-LABEL: define {{.*}}i32 @ignore_result_assign() +// OGCG: %[[RETVAL:.*]] = alloca i32 +// OGCG: %[[P:.*]] = alloca ptr +// OGCG: %[[Q:.*]] = alloca ptr +// OGCG: store ptr null, ptr %[[Q]] +// OGCG: %[[Q_VAL:.*]] = load ptr, ptr %[[Q]] +// OGCG: store ptr %[[Q_VAL]], ptr %[[P]] +// OGCG: %[[TOBOOL:.*]] = icmp ne ptr %[[Q_VAL]], null +// OGCG: br i1 %[[TOBOOL]], label %[[IF_THEN:.*]], label %[[IF_END:.*]] +// OGCG: [[IF_THEN]]: +// OGCG: store i32 1, ptr %[[RETVAL]] +// OGCG: br label %[[RETURN:.*]] +// OGCG: [[IF_END]]: +// OGCG: store i32 0, ptr %[[RETVAL]] +// OGCG: br label %[[RETURN]] +// OGCG: [[RETURN]]: +// OGCG: %{{.*}} = load i32, ptr %[[RETVAL]] +// OGCG: ret i32 |
| @llvm/pr-subscribers-clangir Author: Morris Hafner (mmha) ChangesWe are missing a couple of cases were we are not supposed to ignore assignment results but did so, which results in compiler crashes. Fix that. Full diff: https://github.com/llvm/llvm-project/pull/166118.diff 2 Files Affected:
diff --git a/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp b/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp index 119314fe27dce..4156d8cb873da 100644 --- a/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp +++ b/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp @@ -78,7 +78,7 @@ struct BinOpInfo { class ScalarExprEmitter : public StmtVisitor<ScalarExprEmitter, mlir::Value> { CIRGenFunction &cgf; CIRGenBuilderTy &builder; - bool ignoreResultAssign; + bool ignoreResultAssign = false; public: ScalarExprEmitter(CIRGenFunction &cgf, CIRGenBuilderTy &builder) @@ -221,6 +221,8 @@ class ScalarExprEmitter : public StmtVisitor<ScalarExprEmitter, mlir::Value> { } mlir::Value VisitArraySubscriptExpr(ArraySubscriptExpr *e) { + ignoreResultAssign = false; + if (e->getBase()->getType()->isVectorType()) { assert(!cir::MissingFeatures::scalableVectors()); @@ -839,6 +841,7 @@ class ScalarExprEmitter : public StmtVisitor<ScalarExprEmitter, mlir::Value> { BinOpInfo emitBinOps(const BinaryOperator *e, QualType promotionType = QualType()) { + ignoreResultAssign = false; BinOpInfo result; result.lhs = cgf.emitPromotedScalarExpr(e->getLHS(), promotionType); result.rhs = cgf.emitPromotedScalarExpr(e->getRHS(), promotionType); @@ -924,6 +927,7 @@ class ScalarExprEmitter : public StmtVisitor<ScalarExprEmitter, mlir::Value> { #undef HANDLEBINOP mlir::Value emitCmp(const BinaryOperator *e) { + ignoreResultAssign = false; const mlir::Location loc = cgf.getLoc(e->getExprLoc()); mlir::Value result; QualType lhsTy = e->getLHS()->getType(); @@ -2054,6 +2058,11 @@ mlir::Value ScalarExprEmitter::VisitMemberExpr(MemberExpr *e) { mlir::Value ScalarExprEmitter::VisitInitListExpr(InitListExpr *e) { const unsigned numInitElements = e->getNumInits(); + [[maybe_unused]] const bool ignore = std::exchange(ignoreResultAssign, false); + assert((ignore == false || + (numInitElements == 0 && e->getType()->isVoidType())) && + "init list ignored"); + if (e->hadArrayRangeDesignator()) { cgf.cgm.errorNYI(e->getSourceRange(), "ArrayRangeDesignator"); return {}; diff --git a/clang/test/CIR/CodeGen/binassign.c b/clang/test/CIR/CodeGen/binassign.c index 44c54b4a2969a..966c0e921f346 100644 --- a/clang/test/CIR/CodeGen/binassign.c +++ b/clang/test/CIR/CodeGen/binassign.c @@ -100,3 +100,67 @@ void binary_assign_struct() { // OGCG: call void @llvm.memcpy.p0.p0.i64(ptr align 4 %[[LS_PTR]], ptr align 4 @gs, i64 8, i1 false) // OGCG: call void @llvm.memcpy.p0.p0.i64(ptr align 4 %[[LSV_PTR]], ptr align 4 @gsv, i64 8, i1 true) // OGCG: ret void + +int ignore_result_assign() { + int *p, *q = 0; + if(p = q) + return 1; + return 0; +} + +// CIR-LABEL: cir.func{{.*}} @ignore_result_assign() -> !s32i +// CIR: %[[RETVAL:.*]] = cir.alloca !s32i, !cir.ptr<!s32i>, ["__retval"] +// CIR: %[[P:.*]] = cir.alloca !cir.ptr<!s32i>, !cir.ptr<!cir.ptr<!s32i>>, ["p"] +// CIR: %[[Q:.*]] = cir.alloca !cir.ptr<!s32i>, !cir.ptr<!cir.ptr<!s32i>>, ["q", init] +// CIR: %[[NULL:.*]] = cir.const #cir.ptr<null> : !cir.ptr<!s32i> +// CIR: cir.store{{.*}} %[[NULL]], %[[Q]] : !cir.ptr<!s32i>, !cir.ptr<!cir.ptr<!s32i>> +// CIR: %[[Q_VAL:.*]] = cir.load{{.*}} %[[Q]] : !cir.ptr<!cir.ptr<!s32i>>, !cir.ptr<!s32i> +// CIR: cir.store{{.*}} %[[Q_VAL]], %[[P]] : !cir.ptr<!s32i>, !cir.ptr<!cir.ptr<!s32i>> +// CIR: %[[COND:.*]] = cir.cast ptr_to_bool %[[Q_VAL]] : !cir.ptr<!s32i> -> !cir.bool +// CIR: cir.if %[[COND]] { +// CIR: %[[ONE:.*]] = cir.const #cir.int<1> : !s32i +// CIR: cir.store %[[ONE]], %[[RETVAL]] : !s32i, !cir.ptr<!s32i> +// CIR: %{{.*}} = cir.load %[[RETVAL]] : !cir.ptr<!s32i>, !s32i +// CIR: cir.return +// CIR: } +// CIR: %[[ZERO:.*]] = cir.const #cir.int<0> : !s32i +// CIR: cir.store %[[ZERO]], %[[RETVAL]] : !s32i, !cir.ptr<!s32i> +// CIR: %{{.*}} = cir.load %[[RETVAL]] : !cir.ptr<!s32i>, !s32i +// CIR: cir.return + +// LLVM-LABEL: define {{.*}}i32 @ignore_result_assign() +// LLVM: %[[RETVAL_PTR:.*]] = alloca i32 +// LLVM: %[[P_PTR:.*]] = alloca ptr +// LLVM: %[[Q_PTR:.*]] = alloca ptr +// LLVM: store ptr null, ptr %[[Q_PTR]] +// LLVM: %[[Q_VAL:.*]] = load ptr, ptr %[[Q_PTR]] +// LLVM: store ptr %[[Q_VAL]], ptr %[[P_PTR]] +// LLVM: %[[CMP:.*]] = icmp ne ptr %[[Q_VAL]], null +// LLVM: br i1 %[[CMP]], label %[[THEN:.*]], label %[[ELSE:.*]] +// LLVM: [[THEN]]: +// LLVM: store i32 1, ptr %[[RETVAL_PTR]] +// LLVM: %{{.*}} = load i32, ptr %[[RETVAL_PTR]] +// LLVM: ret i32 +// LLVM: [[ELSE]]: +// LLVM: store i32 0, ptr %[[RETVAL_PTR]] +// LLVM: %{{.*}} = load i32, ptr %[[RETVAL_PTR]] +// LLVM: ret i32 + +// OGCG-LABEL: define {{.*}}i32 @ignore_result_assign() +// OGCG: %[[RETVAL:.*]] = alloca i32 +// OGCG: %[[P:.*]] = alloca ptr +// OGCG: %[[Q:.*]] = alloca ptr +// OGCG: store ptr null, ptr %[[Q]] +// OGCG: %[[Q_VAL:.*]] = load ptr, ptr %[[Q]] +// OGCG: store ptr %[[Q_VAL]], ptr %[[P]] +// OGCG: %[[TOBOOL:.*]] = icmp ne ptr %[[Q_VAL]], null +// OGCG: br i1 %[[TOBOOL]], label %[[IF_THEN:.*]], label %[[IF_END:.*]] +// OGCG: [[IF_THEN]]: +// OGCG: store i32 1, ptr %[[RETVAL]] +// OGCG: br label %[[RETURN:.*]] +// OGCG: [[IF_END]]: +// OGCG: store i32 0, ptr %[[RETVAL]] +// OGCG: br label %[[RETURN]] +// OGCG: [[RETURN]]: +// OGCG: %{{.*}} = load i32, ptr %[[RETVAL]] +// OGCG: ret i32 |
andykaylor approved these changes Nov 3, 2025
bcardosolopes approved these changes Nov 4, 2025
| CIRGenFunction &cgf; | ||
| CIRGenBuilderTy &builder; | ||
| bool ignoreResultAssign; | ||
| bool ignoreResultAssign = false; |
There was a problem hiding this comment.
This sounds like a good change regardless, but I also wonder if this is a good time to introduce TestAndClearIgnoreResultAssign? It usually cleanup this state, but looks like it hasn't been upstreamed just yet
There was a problem hiding this comment.
We had a discussion about this early on in the upstreaming, and we decided not to upstream TestAndClearIgnoreResultAssign specifically because it was so often obscuring the fact that we just wanted to set it to false. I tried to find the discussion, but so far with no luck. We're using std::exchange in the few places where we actually want the value before clearing it.
There was a problem hiding this comment.
Right, I forgot about it. Is there a comment in code anywhere to this effect? Might be handy to have that when comparing OGCG with CIRGen.
| I noticed that we were also not ignoring expressions in IgnoredExprs, making this hard to test. I changed this, too and added a few tests to ensure assignments do not get ignored. |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
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.
We are missing a couple of cases were we are not supposed to ignore assignment results but did so, which results in compiler crashes. Fix that.