Skip to content

[CIR] Fix assignment ignore in ScalarExprEmitter #166118

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 5, 2025
+126 −6
Merged

[CIR] Fix assignment ignore in ScalarExprEmitter #166118

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
16947a9
[CIR] Fix assignment ignore in ScalarExprEmitter
mmha Nov 3, 2025
af34186
Ignore expressions in IgnoreExprs, expand testing
mmha Nov 4, 2025
7bafe49
Add comment explaining the absence of TestAndClearIgnoreResultAssign
mmha Nov 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ struct BinOpInfo {
class ScalarExprEmitter : public StmtVisitor<ScalarExprEmitter, mlir::Value> {
CIRGenFunction &cgf;
CIRGenBuilderTy &builder;
bool ignoreResultAssign;
bool ignoreResultAssign = false;
Copy link
Member

@bcardosolopes bcardosolopes Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@andykaylor andykaylor Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@bcardosolopes bcardosolopes Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@mmha mmha Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just added a comment for this.


public:
ScalarExprEmitter(CIRGenFunction &cgf, CIRGenBuilderTy &builder)
Expand Down Expand Up @@ -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());

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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 {};
Expand Down
64 changes: 64 additions & 0 deletions clang/test/CIR/CodeGen/binassign.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading