[CIR] Implement BinComma Expr for AggregateExpr

[AmrDeveloper]

Implement the BinComma Expr support for AggregateExpr

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clangir

Author: Amr Hesham (AmrDeveloper)

Changes

Implement the BinComma Expr support for AggregateExpr

---

Full diff: https://github.com/llvm/llvm-project/pull/161823.diff

2 Files Affected:

• (modified) clang/lib/CIR/CodeGen/CIRGenExprAggregate.cpp (+2-1)
• (modified) clang/test/CIR/CodeGen/struct.cpp (+19)

diff --git a/clang/lib/CIR/CodeGen/CIRGenExprAggregate.cpp b/clang/lib/CIR/CodeGen/CIRGenExprAggregate.cpp
index 1e987f3bedc7e..d3540a2649cb4 100644
--- a/clang/lib/CIR/CodeGen/CIRGenExprAggregate.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenExprAggregate.cpp
@@ -184,7 +184,8 @@ class AggExprEmitter : public StmtVisitor<AggExprEmitter> {
     cgf.cgm.errorNYI(e->getSourceRange(), "AggExprEmitter: VisitBinAssign");
   }
   void VisitBinComma(const BinaryOperator *e) {
-    cgf.cgm.errorNYI(e->getSourceRange(), "AggExprEmitter: VisitBinComma");
+    cgf.emitIgnoredExpr(e->getLHS());
+    Visit(e->getRHS());
   }
   void VisitBinCmp(const BinaryOperator *e) {
     cgf.cgm.errorNYI(e->getSourceRange(), "AggExprEmitter: VisitBinCmp");
diff --git a/clang/test/CIR/CodeGen/struct.cpp b/clang/test/CIR/CodeGen/struct.cpp
index 96db82a89977c..a3d35b834bafc 100644
--- a/clang/test/CIR/CodeGen/struct.cpp
+++ b/clang/test/CIR/CodeGen/struct.cpp
@@ -183,3 +183,22 @@ void generic_selection() {
 // OGCG:   %[[C_ADDR:.*]] = alloca i32, align 4
 // OGCG:   %[[D_ADDR:.*]] = alloca %struct.CompleteS, align 4
 // OGCG:   call void @llvm.memcpy.p0.p0.i64(ptr align 4 %[[D_ADDR]], ptr align 4 %[[A_ADDR]], i64 8, i1 false)
+
+void bin_comma() { 
+  CompleteS a = (CompleteS(), CompleteS());
+}
+
+// CIR: %[[A_ADDR:.*]] = cir.alloca !rec_CompleteS, !cir.ptr<!rec_CompleteS>, ["a", init]
+// CIR: %[[TMP_ADDR:.*]] = cir.alloca !rec_CompleteS, !cir.ptr<!rec_CompleteS>, ["agg.tmp0"]
+// CIR: %[[ZERO:.*]] = cir.const #cir.zero : !rec_CompleteS
+// CIR: cir.store{{.*}} %[[ZERO]], %[[TMP_ADDR]] : !rec_CompleteS, !cir.ptr<!rec_CompleteS>
+// CIR: %[[ZERO:.*]] = cir.const #cir.zero : !rec_CompleteS
+// CIR: cir.store{{.*}} %[[ZERO]], %[[A_ADDR]] : !rec_CompleteS, !cir.ptr<!rec_CompleteS>
+
+// LLVM: %[[A_ADDR:.*]] = alloca %struct.CompleteS, i64 1, align 4
+// LLVM: %[[TMP_ADDR:.*]] = alloca %struct.CompleteS, i64 1, align 4
+// LLVM: store %struct.CompleteS zeroinitializer, ptr %[[TMP_ADDR]], align 4
+// LLVM: store %struct.CompleteS zeroinitializer, ptr %[[A_ADDR]], align 4
+
+// OGCG: %[[A_ADDR:.*]] = alloca %struct.CompleteS, align 4
+// OGCG: call void @llvm.memset.p0.i64(ptr align 4 %[[A_ADDR]], i8 0, i64 8, i1 false)

[andykaylor]

This looks fine, but I have a question for possible future follow-up.

[andykaylor]

Do you know why OGCG doesn't emit the temporary? Is it checking somewhere for a trivial constructor and not emitting unused temporaries, or does it get deleted somewhere?

[AmrDeveloper]

Not yet, I will check what and where the OGCG do that because in the emitter it just similar to CIR

[AmrDeveloper]

The difference is from CodeGenFunction::EmitAnyExpr there is extra param ignoreResult and the temporary is emitted only if ignoreResult is false which is false by default

OGCG:

llvm-project/clang/lib/CodeGen/CGExpr.cpp

Line 273 in eabfed8

if (!ignoreResult && aggSlot.isIgnored())

llvm-project/clang/lib/CIR/CodeGen/CIRGenExpr.cpp

Line 1630 in eabfed8

if (aggSlot.isIgnored())

[AmrDeveloper]

Actually, we have this parameter in the incubator, but it's not upstreamed yet. I will handle it in a follow-up PR to check other pleases also

[bcardosolopes]

LGTM, please thread in the extra parameter soon-ish in follow up work.