[CIR] Upstream support for data member comparison

[andykaylor]

This adds support for handling data member pointer comparisons in CIR.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clangir

Author: Andy Kaylor (andykaylor)

Changes

This adds support for handling data member pointer comparisons in CIR.

---

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

4 Files Affected:

• (modified) clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRCXXABI.h (+4)
• (modified) clang/lib/CIR/Dialect/Transforms/TargetLowering/LowerItaniumCXXABI.cpp (+12)
• (modified) clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp (+10-1)
• (added) clang/test/CIR/CodeGen/pointer-to-data-member-cmp.cpp (+58)

diff --git a/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRCXXABI.h b/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRCXXABI.h
index 90f0ac3478f9d..a2439f1c24f10 100644
--- a/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRCXXABI.h
+++ b/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRCXXABI.h
@@ -53,6 +53,10 @@ class CIRCXXABI {
   lowerGetRuntimeMember(cir::GetRuntimeMemberOp op, mlir::Type loweredResultTy,
                         mlir::Value loweredAddr, mlir::Value loweredMember,
                         mlir::OpBuilder &builder) const = 0;
+
+  virtual mlir::Value lowerDataMemberCmp(cir::CmpOp op, mlir::Value loweredLhs,
+                                         mlir::Value loweredRhs,
+                                         mlir::OpBuilder &builder) const = 0;
 };
 
 /// Creates an Itanium-family ABI.
diff --git a/clang/lib/CIR/Dialect/Transforms/TargetLowering/LowerItaniumCXXABI.cpp b/clang/lib/CIR/Dialect/Transforms/TargetLowering/LowerItaniumCXXABI.cpp
index ce4b0c7e92d09..abffb193798cc 100644
--- a/clang/lib/CIR/Dialect/Transforms/TargetLowering/LowerItaniumCXXABI.cpp
+++ b/clang/lib/CIR/Dialect/Transforms/TargetLowering/LowerItaniumCXXABI.cpp
@@ -47,6 +47,10 @@ class LowerItaniumCXXABI : public CIRCXXABI {
   lowerGetRuntimeMember(cir::GetRuntimeMemberOp op, mlir::Type loweredResultTy,
                         mlir::Value loweredAddr, mlir::Value loweredMember,
                         mlir::OpBuilder &builder) const override;
+
+  mlir::Value lowerDataMemberCmp(cir::CmpOp op, mlir::Value loweredLhs,
+                                 mlir::Value loweredRhs,
+                                 mlir::OpBuilder &builder) const override;
 };
 
 } // namespace
@@ -108,4 +112,12 @@ mlir::Operation *LowerItaniumCXXABI::lowerGetRuntimeMember(
                              cir::CastKind::bitcast, memberBytesPtr);
 }
 
+mlir::Value
+LowerItaniumCXXABI::lowerDataMemberCmp(cir::CmpOp op, mlir::Value loweredLhs,
+                                       mlir::Value loweredRhs,
+                                       mlir::OpBuilder &builder) const {
+  return cir::CmpOp::create(builder, op.getLoc(), op.getKind(), loweredLhs,
+                            loweredRhs);
+}
+
 } // namespace cir
diff --git a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
index 00307df62ce5a..026a165782c44 100644
--- a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
+++ b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
@@ -2559,8 +2559,17 @@ mlir::LogicalResult CIRToLLVMCmpOpLowering::matchAndRewrite(
     mlir::ConversionPatternRewriter &rewriter) const {
   mlir::Type type = cmpOp.getLhs().getType();
 
-  assert(!cir::MissingFeatures::dataMemberType());
   assert(!cir::MissingFeatures::methodType());
+  if (mlir::isa<cir::DataMemberType>(type)) {
+    assert(lowerMod && "lowering module is not available");
+
+    mlir::Value loweredResult;
+    loweredResult = lowerMod->getCXXABI().lowerDataMemberCmp(
+        cmpOp, adaptor.getLhs(), adaptor.getRhs(), rewriter);
+
+    rewriter.replaceOp(cmpOp, loweredResult);
+    return mlir::success();
+  }
 
   if (mlir::isa<cir::IntType, mlir::IntegerType>(type)) {
     bool isSigned = mlir::isa<cir::IntType>(type)
diff --git a/clang/test/CIR/CodeGen/pointer-to-data-member-cmp.cpp b/clang/test/CIR/CodeGen/pointer-to-data-member-cmp.cpp
new file mode 100644
index 0000000000000..fa3491252cb5f
--- /dev/null
+++ b/clang/test/CIR/CodeGen/pointer-to-data-member-cmp.cpp
@@ -0,0 +1,58 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++17 -fclangir -emit-cir %s -o %t.cir
+// RUN: FileCheck --input-file=%t.cir --check-prefix=CIR %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++17 -fclangir -emit-llvm %s -o %t-cir.ll
+// RUN: FileCheck --input-file=%t-cir.ll --check-prefix=LLVM %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++17 -emit-llvm %s -o %t.ll
+// RUN: FileCheck --input-file=%t.ll --check-prefix=OGCG %s
+
+struct Foo {
+  int a;
+};
+
+struct Bar {
+  int a;
+};
+
+bool eq(int Foo::*x, int Foo::*y) {
+  return x == y;
+}
+
+// CIR-LABEL: @_Z2eqM3FooiS0_
+//      CIR:   %[[#x:]] = cir.load{{.*}} %{{.+}} : !cir.ptr<!cir.data_member<!s32i in !rec_Foo>>, !cir.data_member<!s32i in !rec_Foo>
+// CIR-NEXT:   %[[#y:]] = cir.load{{.*}} %{{.+}} : !cir.ptr<!cir.data_member<!s32i in !rec_Foo>>, !cir.data_member<!s32i in !rec_Foo>
+// CIR-NEXT:   %{{.+}} = cir.cmp(eq, %[[#x]], %[[#y]]) : !cir.data_member<!s32i in !rec_Foo>, !cir.bool
+//      CIR: }
+
+// LLVM-LABEL: @_Z2eqM3FooiS0_
+//      LLVM:   %[[#x:]] = load i64, ptr %{{.+}}, align 8
+// LLVM-NEXT:   %[[#y:]] = load i64, ptr %{{.+}}, align 8
+// LLVM-NEXT:   %{{.+}} = icmp eq i64 %[[#x]], %[[#y]]
+//      LLVM: }
+
+// OGCG-LABEL: @_Z2eqM3FooiS0_
+//      OGCG:   %[[#x:]] = load i64, ptr %{{.+}}, align 8
+// OGCG-NEXT:   %[[#y:]] = load i64, ptr %{{.+}}, align 8
+// OGCG-NEXT:   %{{.+}} = icmp eq i64 %[[#x]], %[[#y]]
+//      OGCG: }
+
+bool ne(int Foo::*x, int Foo::*y) {
+  return x != y;
+}
+
+// CIR-LABEL: @_Z2neM3FooiS0_
+//      CIR:   %[[#x:]] = cir.load{{.*}} %{{.+}} : !cir.ptr<!cir.data_member<!s32i in !rec_Foo>>, !cir.data_member<!s32i in !rec_Foo>
+// CIR-NEXT:   %[[#y:]] = cir.load{{.*}} %{{.+}} : !cir.ptr<!cir.data_member<!s32i in !rec_Foo>>, !cir.data_member<!s32i in !rec_Foo>
+// CIR-NEXT:   %{{.+}} = cir.cmp(ne, %[[#x]], %[[#y]]) : !cir.data_member<!s32i in !rec_Foo>, !cir.bool
+//      CIR: }
+
+// LLVM-LABEL: @_Z2neM3FooiS0_
+//      LLVM:   %[[#x:]] = load i64, ptr %{{.+}}, align 8
+// LLVM-NEXT:   %[[#y:]] = load i64, ptr %{{.+}}, align 8
+// LLVM-NEXT:   %{{.+}} = icmp ne i64 %[[#x]], %[[#y]]
+//      LLVM: }
+
+// OGCG-LABEL: @_Z2neM3FooiS0_
+//      OGCG:   %[[#x:]] = load i64, ptr %{{.+}}, align 8
+// OGCG-NEXT:   %[[#y:]] = load i64, ptr %{{.+}}, align 8
+// OGCG-NEXT:   %{{.+}} = icmp ne i64 %[[#x]], %[[#y]]
+//      OGCG: }

[xlauko]

Is there a reason to create cir::CmpOp and not LLVM::ICmpOp equivalent? I believe loweredLhs and loweredRhs are already LLVM dialect values, so technically we gen invalid intermediate IR.

[andykaylor]

That's a good question. It may be that at some point in the past there was an intention for this lowering to happen as a CIR-to-CIR transform, similar to LoweringPrepare. I think it would make sense for this (and the rest of the CXXABI handling) to be something we could do independent of lowering to the LLVM dialect, but it may be better to implement it as you have suggested as long as it is part of the lowering to LLVM IR. We can always switch it back to this form if we move this to a standalone pass.

[Lancern]

It may be that at some point in the past there was an intention for this lowering to happen as a CIR-to-CIR transform, similar to LoweringPrepare.

I have an ongoing PR in the incubator llvm/clangir#1471 which tries to add a CIR-to-CIR pass dedicated to ABI lowering. It's a bit out of sync now because recently I do not get enough bandwidth to work on that. I feel it's sub-optimal to mix ABI lowering work in the process of LLVMLowering like what we're doing now, because:

1. It complicates the lowering process and confuses people. I could remember at least three times reviewers were confused by patches to LLVMLowering which also contain ABI lowering work.
2. It also makes debugging trickier because while it's easy to dump IRs between passes, it's relatively harder to see what happens during a pass.
3. Besides LLVMLowering, we also have a MLIRLowering route. Writing ABI lowering code in LLVMLowering does not help the MLIRLowering route, and it's likely we have to re-implement ABI lowering code in MLIRLowering again. Moving ABI lowering code to a dedicated pass before LLVMLowering also benefits MLIRLowering.

[andykaylor]

@Lancern Could you at least rebase your incubator patch? In the current state it's impossible to even see what you were doing.

There's little enough done upstream for the pointer-to-member handling and LowerModule implementation that it wouldn't be very difficult to refactor it into a standalone CIR-to-CIR pass. That's probably easier than trying to rewrite everything in the incubator, and we can just transition things there as they are upstreamed and backport to the incubator just by removing things as they are implemented.

[andykaylor]

@Lancern I realized that I could just look at the main commit from your PR and ignore everything that got piled on top of it through incubator rebases. I was able to pretty much use your lowering pass structure as it was, just removing the parts that we weren't ready for upstream. I've just posted #172133 to do this.

If we all agree on this direction, I'll wait for that to land and then rebase this and the other related in-flight PRs on it.

[Lancern]

I'll later rebase my patch onto the latest incubator main.

[Lancern]

Rebased.