Skip to content
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.

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

[mlir][EmitC] Add member access ops #98460

Merged
merged 1 commit into from
Jul 13, 2024
Merged

[mlir][EmitC] Add member access ops #98460

merged 1 commit into from
Jul 13, 2024

Conversation

marbre
Copy link
Member

@marbre marbre commented Jul 11, 2024

This adds an emitc.member and emitc.member_of_ptr operation for the corresponding member access operators. Furthermore, emitc.assign is adjusted to be used with the member acces operators.

@llvmbot
Copy link
Collaborator

llvmbot commented Jul 11, 2024

@llvm/pr-subscribers-mlir-emitc

@llvm/pr-subscribers-mlir

Author: Marius Brehler (marbre)

Changes

This adds an emitc.member and emitc.member_of_ptr operation for the corresponding member access operators. Furthermore, emitc.assign is adjusted to be used with the member acces operators.


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

6 Files Affected:

  • (modified) mlir/include/mlir/Dialect/EmitC/IR/EmitC.td (+42)
  • (modified) mlir/lib/Dialect/EmitC/IR/EmitC.cpp (+4-3)
  • (modified) mlir/lib/Target/Cpp/TranslateToCpp.cpp (+32-2)
  • (modified) mlir/test/Dialect/EmitC/invalid_ops.mlir (+17-1)
  • (modified) mlir/test/Dialect/EmitC/ops.mlir (+7)
  • (added) mlir/test/Target/Cpp/member.mlir (+34)
diff --git a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
index 452302c565139..2ee51f430deaf 100644
--- a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
+++ b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
@@ -908,6 +908,48 @@ def EmitC_SubOp : EmitC_BinaryOp<"sub", [CExpression]> {
   let hasVerifier = 1;
 }
 
+def EmitC_MemberOp : EmitC_Op<"member"> {
+  let summary = "Member operation";
+  let description = [{
+    With the `member` operation the member access operator `.` can be
+    applied.
+
+    Example:
+
+    ```mlir
+    %0 = "emitc.member" (%arg0) {member = "a"}
+        : (!emitc.opaque<"mystruct">) -> i32
+    ```
+  }];
+
+  let arguments = (ins
+    Arg<StrAttr, "the member to access">:$member,
+    EmitC_OpaqueType:$operand
+  );
+  let results = (outs EmitCType);
+}
+
+def EmitC_MemberOfPtrOp : EmitC_Op<"member_of_ptr"> {
+  let summary = "Member operation";
+  let description = [{
+    With the `member_of_ptr` operation the member access operator `->`
+    can be applied.
+
+    Example:
+
+    ```mlir
+    %0 = "emitc.member_of_ptr" (%arg0) {member = "a"}
+        : (!emitc.ptr<!emitc.opaque<"mystruct">>) -> i32
+    ```
+  }];
+
+  let arguments = (ins
+    Arg<StrAttr, "the member to access">:$member,
+    AnyTypeOf<[EmitC_OpaqueType,EmitC_PointerType]>:$operand
+  );
+  let results = (outs EmitCType);
+}
+
 def EmitC_ConditionalOp : EmitC_Op<"conditional",
     [AllTypesMatch<["true_value", "false_value", "result"]>, CExpression]> {
   let summary = "Conditional (ternary) operation";
diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
index 9f99eb1233cb1..70e3e728e0195 100644
--- a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
+++ b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
@@ -213,10 +213,11 @@ LogicalResult emitc::AssignOp::verify() {
   Value variable = getVar();
   Operation *variableDef = variable.getDefiningOp();
   if (!variableDef ||
-      !llvm::isa<emitc::GetGlobalOp, emitc::SubscriptOp, emitc::VariableOp>(
-          variableDef))
+      !llvm::isa<emitc::GetGlobalOp, emitc::MemberOp, emitc::MemberOfPtrOp,
+                 emitc::SubscriptOp, emitc::VariableOp>(variableDef))
     return emitOpError() << "requires first operand (" << variable
-                         << ") to be a get_global, subscript or variable";
+                         << ") to be a get_global, member, member of pointer, "
+                            "subscript or variable";
 
   Value value = getValue();
   if (variable.getType() != value.getType())
diff --git a/mlir/lib/Target/Cpp/TranslateToCpp.cpp b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
index eda8d5c9ad8cb..57ebfa9692083 100644
--- a/mlir/lib/Target/Cpp/TranslateToCpp.cpp
+++ b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
@@ -183,6 +183,12 @@ struct CppEmitter {
   // Returns the textual representation of a subscript operation.
   std::string getSubscriptName(emitc::SubscriptOp op);
 
+  // Returns the textual representation of a member (of object) operation.
+  std::string getOperandAndMemberName(emitc::MemberOp op);
+
+  // Returns the textual representation of a member of pointer operation.
+  std::string getOperandAndMemberName(emitc::MemberOfPtrOp op);
+
   /// Return the existing or a new label of a Block.
   StringRef getOrCreateName(Block &block);
 
@@ -278,8 +284,8 @@ struct CppEmitter {
 
 /// Determine whether expression \p op should be emitted in a deferred way.
 static bool hasDeferredEmission(Operation *op) {
-  return isa_and_nonnull<emitc::GetGlobalOp, emitc::LiteralOp,
-                         emitc::SubscriptOp>(op);
+  return isa_and_nonnull<emitc::GetGlobalOp, emitc::LiteralOp, emitc::MemberOp,
+                         emitc::MemberOfPtrOp, emitc::SubscriptOp>(op);
 }
 
 /// Determine whether expression \p expressionOp should be emitted inline, i.e.
@@ -1125,6 +1131,22 @@ std::string CppEmitter::getSubscriptName(emitc::SubscriptOp op) {
   return out;
 }
 
+std::string CppEmitter::getOperandAndMemberName(emitc::MemberOp op) {
+  std::string out;
+  llvm::raw_string_ostream ss(out);
+  ss << getOrCreateName(op.getOperand());
+  ss << "." << op.getMember();
+  return out;
+}
+
+std::string CppEmitter::getOperandAndMemberName(emitc::MemberOfPtrOp op) {
+  std::string out;
+  llvm::raw_string_ostream ss(out);
+  ss << getOrCreateName(op.getOperand());
+  ss << "->" << op.getMember();
+  return out;
+}
+
 void CppEmitter::cacheDeferredOpResult(Value value, StringRef str) {
   if (!valueMapper.count(value))
     valueMapper.insert(value, str.str());
@@ -1501,6 +1523,14 @@ LogicalResult CppEmitter::emitOperation(Operation &op, bool trailingSemicolon) {
             cacheDeferredOpResult(op.getResult(), op.getValue());
             return success();
           })
+          .Case<emitc::MemberOp>([&](auto op) {
+            cacheDeferredOpResult(op.getResult(), getOperandAndMemberName(op));
+            return success();
+          })
+          .Case<emitc::MemberOfPtrOp>([&](auto op) {
+            cacheDeferredOpResult(op.getResult(), getOperandAndMemberName(op));
+            return success();
+          })
           .Case<emitc::SubscriptOp>([&](auto op) {
             cacheDeferredOpResult(op.getResult(), getSubscriptName(op));
             return success();
diff --git a/mlir/test/Dialect/EmitC/invalid_ops.mlir b/mlir/test/Dialect/EmitC/invalid_ops.mlir
index e9b11421882f9..4181b726593e4 100644
--- a/mlir/test/Dialect/EmitC/invalid_ops.mlir
+++ b/mlir/test/Dialect/EmitC/invalid_ops.mlir
@@ -235,7 +235,7 @@ func.func @test_misplaced_yield() {
 // -----
 
 func.func @test_assign_to_non_variable(%arg1: f32, %arg2: f32) {
-  // expected-error @+1 {{'emitc.assign' op requires first operand (<block argument> of type 'f32' at index: 1) to be a get_global, subscript or variable}}
+  // expected-error @+1 {{'emitc.assign' op requires first operand (<block argument> of type 'f32' at index: 1) to be a get_global, member, member of pointer, subscript or variable}}
   emitc.assign %arg1 : f32 to %arg2 : f32
   return
 }
@@ -450,3 +450,19 @@ func.func @use_global() {
   %0 = emitc.get_global @myglobal : f32
   return
 }
+
+// -----
+
+func.func @member(%arg0: i32) {
+  // expected-error @+1 {{'emitc.member' op operand #0 must be EmitC opaque type, but got 'i32'}}
+  %0 = "emitc.member" (%arg0) {member = "a"} : (i32) -> i32
+  return
+}
+
+// -----
+
+func.func @member_of_ptr(%arg0: i32) {
+  // expected-error @+1 {{'emitc.member_of_ptr' op operand #0 must be EmitC opaque type or EmitC pointer type, but got 'i32}}
+  %0 = "emitc.member_of_ptr" (%arg0) {member = "a"} : (i32) -> i32
+  return
+}
diff --git a/mlir/test/Dialect/EmitC/ops.mlir b/mlir/test/Dialect/EmitC/ops.mlir
index 1d3ca5c9bc939..20ac077e4402b 100644
--- a/mlir/test/Dialect/EmitC/ops.mlir
+++ b/mlir/test/Dialect/EmitC/ops.mlir
@@ -254,3 +254,10 @@ func.func @assign_global(%arg0 : i32) {
   emitc.assign %arg0 : i32 to %0 : i32
   return
 }
+
+func.func @member_access(%arg0: !emitc.opaque<"mystruct">, %arg1: !emitc.opaque<"mystruct_ptr">, %arg2: !emitc.ptr<!emitc.opaque<"mystruct">>) {
+  %0 = "emitc.member" (%arg0) {member = "a"} : (!emitc.opaque<"mystruct">) -> i32
+  %1 = "emitc.member_of_ptr" (%arg1) {member = "a"} : (!emitc.opaque<"mystruct_ptr">) -> i32
+  %2 = "emitc.member_of_ptr" (%arg2) {member = "a"} : (!emitc.ptr<!emitc.opaque<"mystruct">>) -> i32
+  return
+}
diff --git a/mlir/test/Target/Cpp/member.mlir b/mlir/test/Target/Cpp/member.mlir
new file mode 100644
index 0000000000000..1b4a3dcab879d
--- /dev/null
+++ b/mlir/test/Target/Cpp/member.mlir
@@ -0,0 +1,34 @@
+// RUN: mlir-translate -mlir-to-cpp %s | FileCheck %s -check-prefix=CPP-DEFAULT
+
+func.func @member(%arg0: !emitc.opaque<"mystruct">, %arg1: i32) {
+  %0 = "emitc.member" (%arg0) {member = "a"} : (!emitc.opaque<"mystruct">) -> i32
+  emitc.assign %arg1 : i32 to %0 : i32 
+
+  %1 = "emitc.member" (%arg0) {member = "b"} : (!emitc.opaque<"mystruct">) -> i32
+  %2 = "emitc.variable"() <{value = #emitc.opaque<"">}> : () -> i32
+  emitc.assign %1 : i32 to %2 : i32
+
+  return
+}
+
+// CPP-DEFAULT: void member(mystruct [[V0:[^ ]*]], int32_t [[V1:[^ ]*]]) {
+// CPP-DEFAULT-NEXT: [[V0:[^ ]*]].a = [[V1:[^ ]*]];
+// CPP-DEFAULT-NEXT: int32_t [[V2:[^ ]*]];
+// CPP-DEFAULT-NEXT: [[V2:[^ ]*]] = [[V0:[^ ]*]].b;
+
+
+func.func @member_of_pointer(%arg0: !emitc.ptr<!emitc.opaque<"mystruct">>, %arg1: i32) {
+  %0 = "emitc.member_of_ptr" (%arg0) {member = "a"} : (!emitc.ptr<!emitc.opaque<"mystruct">>) -> i32
+  emitc.assign %arg1 : i32 to %0 : i32
+
+  %1 = "emitc.member_of_ptr" (%arg0) {member = "b"} : (!emitc.ptr<!emitc.opaque<"mystruct">>) -> i32
+  %2 = "emitc.variable"() <{value = #emitc.opaque<"">}> : () -> i32
+  emitc.assign %1 : i32 to %2 : i32
+
+  return
+}
+
+// CPP-DEFAULT: void member_of_pointer(mystruct* [[V0:[^ ]*]], int32_t [[V1:[^ ]*]]) {
+// CPP-DEFAULT-NEXT: [[V0:[^ ]*]]->a = [[V1:[^ ]*]];
+// CPP-DEFAULT-NEXT: int32_t [[V2:[^ ]*]];
+// CPP-DEFAULT-NEXT: [[V2:[^ ]*]] = [[V0:[^ ]*]]->b;

Copy link
Contributor

@aniragil aniragil left a comment

Choose a reason for hiding this comment

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

LGTM (with a nit).
This will address this issue, right?

mlir/lib/Target/Cpp/TranslateToCpp.cpp Outdated Show resolved Hide resolved
@marbre marbre force-pushed the emitc.member branch 2 times, most recently from 6a01d67 to a3db5fa Compare July 13, 2024 09:57
This adds an `emitc.member` and `emitc.member_of_ptr` operation for the
corresponding member access operators. Furthermore, `emitc.assign` is
adjusted to be used with the member acces operators.
@marbre
Copy link
Member Author

marbre commented Jul 13, 2024

LGTM (with a nit). This will address this issue, right?

Thanks for the review. This actually solves the problem you are pointing to, although unfortunately I have missed this thread so far. The motivation rather is to get a few more things right before landing @simon-camp's lvalue patch.

@marbre marbre requested a review from aniragil July 13, 2024 10:04
@marbre marbre merged commit a5cf99d into llvm:main Jul 13, 2024
5 of 6 checks passed
@marbre marbre deleted the emitc.member branch July 13, 2024 12:43
With the `member` operation the member access operator `.` can be
applied.

Example:
Copy link
Member

Choose a reason for hiding this comment

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

This is more just what the ops syntax looks like rather than an example to me. That is, if one didn't know what the op would generate then this doesn't help.

@llvm-ci
Copy link
Collaborator

llvm-ci commented Jul 13, 2024

LLVM Buildbot has detected a new failure on builder mlir-nvidia-gcc7 running on mlir-nvidia while building mlir at step 5 "build-check-mlir-build-only".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/116/builds/1225

Here is the relevant piece of the build log for the reference:

Step 5 (build-check-mlir-build-only) failure: build (failure)
...
47.403 [62/16/4405] Linking CXX static library lib/libMLIRJitRunner.a
49.833 [61/16/4406] Linking CXX executable bin/mlir-cpu-runner
49.850 [60/16/4407] Linking CXX static library lib/libMLIRCAPIConversion.a
49.865 [59/16/4408] Linking CXX static library lib/libMLIRGPUTestPasses.a
51.740 [58/16/4409] Linking CXX executable bin/mlir-translate
54.013 [57/16/4410] Linking CXX executable bin/mlir-vulkan-runner
54.046 [56/16/4411] Linking CXX static library lib/libMyExtensionCh3.a
54.065 [55/16/4412] Building MyExtension.cpp.inc...
54.085 [54/16/4413] Building MyExtension.h.inc...
78.969 [53/16/4414] Building CXX object tools/mlir/lib/CAPI/RegisterEverything/CMakeFiles/obj.MLIRCAPIRegisterEverything.dir/RegisterEverything.cpp.o
FAILED: tools/mlir/lib/CAPI/RegisterEverything/CMakeFiles/obj.MLIRCAPIRegisterEverything.dir/RegisterEverything.cpp.o 
CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /usr/bin/g++-7 -DBUILD_EXAMPLES -DGTEST_HAS_RTTI=0 -DMLIR_CAPI_BUILDING_LIBRARY=1 -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.obj/tools/mlir/lib/CAPI/RegisterEverything -I/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/lib/CAPI/RegisterEverything -I/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.obj/include -I/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/llvm/include -I/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/include -I/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.obj/tools/mlir/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -fno-lifetime-dse -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-uninitialized -Wno-nonnull -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wno-comment -Wno-misleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -Wundef -Wno-unused-but-set-parameter -O3 -DNDEBUG -fvisibility=hidden  -fno-exceptions -funwind-tables -fno-rtti -UNDEBUG -std=c++1z -MD -MT tools/mlir/lib/CAPI/RegisterEverything/CMakeFiles/obj.MLIRCAPIRegisterEverything.dir/RegisterEverything.cpp.o -MF tools/mlir/lib/CAPI/RegisterEverything/CMakeFiles/obj.MLIRCAPIRegisterEverything.dir/RegisterEverything.cpp.o.d -o tools/mlir/lib/CAPI/RegisterEverything/CMakeFiles/obj.MLIRCAPIRegisterEverything.dir/RegisterEverything.cpp.o -c /vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/lib/CAPI/RegisterEverything/RegisterEverything.cpp
g++-7: internal compiler error: Killed (program cc1plus)
Please submit a full bug report,
with preprocessed source if appropriate.
See <file:///usr/share/doc/gcc-7/README.Bugs> for instructions.
command timed out: 1200 seconds without output running [b'ninja', b'-j', b'16', b'check-mlir-build-only'], attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=2167.885475

aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024
This adds an `emitc.member` and `emitc.member_of_ptr` operation for the
corresponding member access operators. Furthermore, `emitc.assign` is
adjusted to be used with the member access operators.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants