Skip to content

Conversation

LekkalaSravya3
Copy link

This PR addresses issue #159746

  • Implemented proper handling of UnrealizedConversionCastOp when lowering from EmitC IR to the target C++ code using the EmitC emitter.
  • Added the tests to ensure the generated c++ code is valid .

Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot
Copy link
Member

llvmbot commented Sep 22, 2025

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-emitc

Author: Lekkala_Sravya-mcw (LekkalaSravya3)

Changes

This PR addresses issue #159746

  • Implemented proper handling of UnrealizedConversionCastOp when lowering from EmitC IR to the target C++ code using the EmitC emitter.
  • Added the tests to ensure the generated c++ code is valid .

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

2 Files Affected:

  • (modified) mlir/lib/Target/Cpp/TranslateToCpp.cpp (+83-1)
  • (added) mlir/test/Target/Cpp/unrealized_conversion_cast.mlir (+15)
diff --git a/mlir/lib/Target/Cpp/TranslateToCpp.cpp b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
index a5bd80e9d6b8b..80d0c83049ac7 100644
--- a/mlir/lib/Target/Cpp/TranslateToCpp.cpp
+++ b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
@@ -782,6 +782,64 @@ static LogicalResult printOperation(CppEmitter &emitter,
   return success();
 }
 
+static LogicalResult printOperation(CppEmitter &emitter,
+                                    mlir::UnrealizedConversionCastOp castOp) {
+  raw_ostream &os = emitter.ostream();
+  Operation &op = *castOp.getOperation();
+
+  if (castOp.getResults().size() != 1 || castOp.getOperands().size() != 1) {
+    return castOp.emitOpError(
+        "expected single result and single operand for conversion cast");
+  }
+
+  Type destType = castOp.getResult(0).getType();
+
+  auto srcPtrType =
+      mlir::dyn_cast<emitc::PointerType>(castOp.getOperand(0).getType());
+  auto destArrayType = mlir::dyn_cast<emitc::ArrayType>(destType);
+
+  if (srcPtrType && destArrayType) {
+
+    // Emit declaration: (*v13)[dims] =
+    if (failed(emitter.emitType(op.getLoc(), destArrayType.getElementType())))
+      return failure();
+    os << " (*" << emitter.getOrCreateName(op.getResult(0)) << ")";
+    for (int64_t dim : destArrayType.getShape())
+      os << "[" << dim << "]";
+    os << " = ";
+
+    os << "(";
+
+    // Emit the C++ type for "datatype (*)[dim1][dim2]..."
+    if (failed(emitter.emitType(op.getLoc(), destArrayType.getElementType())))
+      return failure();
+
+    os << "(*)"; // Pointer to array
+
+    for (int64_t dim : destArrayType.getShape()) {
+      os << "[" << dim << "]";
+    }
+    os << ")";
+    if (failed(emitter.emitOperand(castOp.getOperand(0))))
+      return failure();
+
+    return success();
+  }
+
+  // Fallback to generic C-style cast for other cases
+  if (failed(emitter.emitAssignPrefix(op)))
+    return failure();
+
+  os << "(";
+  if (failed(emitter.emitType(op.getLoc(), destType)))
+    return failure();
+  os << ")";
+  if (failed(emitter.emitOperand(castOp.getOperand(0))))
+    return failure();
+
+  return success();
+}
+
 static LogicalResult printOperation(CppEmitter &emitter,
                                     emitc::ApplyOp applyOp) {
   raw_ostream &os = emitter.ostream();
@@ -1291,7 +1349,29 @@ CppEmitter::CppEmitter(raw_ostream &os, bool declareVariablesAtTop,
 std::string CppEmitter::getSubscriptName(emitc::SubscriptOp op) {
   std::string out;
   llvm::raw_string_ostream ss(out);
-  ss << getOrCreateName(op.getValue());
+  Value baseValue = op.getValue();
+
+  // Check if the baseValue (%arg1) is a result of UnrealizedConversionCastOp
+  // that converts a pointer to an array type.
+  if (auto castOp = dyn_cast_or_null<mlir::UnrealizedConversionCastOp>(
+          baseValue.getDefiningOp())) {
+    auto destArrayType =
+        mlir::dyn_cast<emitc::ArrayType>(castOp.getResult(0).getType());
+    auto srcPtrType =
+        mlir::dyn_cast<emitc::PointerType>(castOp.getOperand(0).getType());
+
+    // If it's a pointer being cast to an array, emit (*varName)
+    if (srcPtrType && destArrayType) {
+      ss << "(*" << getOrCreateName(baseValue) << ")";
+    } else {
+      // Fallback if the cast is not our specific pointer-to-array case
+      ss << getOrCreateName(baseValue);
+    }
+  } else {
+    // Default behavior for a regular array or other base types
+    ss << getOrCreateName(baseValue);
+  }
+
   for (auto index : op.getIndices()) {
     ss << "[" << getOrCreateName(index) << "]";
   }
@@ -1747,6 +1827,8 @@ LogicalResult CppEmitter::emitOperation(Operation &op, bool trailingSemicolon) {
             cacheDeferredOpResult(op.getResult(), getSubscriptName(op));
             return success();
           })
+          .Case<mlir::UnrealizedConversionCastOp>(
+              [&](auto op) { return printOperation(*this, op); })
           .Default([&](Operation *) {
             return op.emitOpError("unable to find printer for op");
           });
diff --git a/mlir/test/Target/Cpp/unrealized_conversion_cast.mlir b/mlir/test/Target/Cpp/unrealized_conversion_cast.mlir
new file mode 100644
index 0000000000000..075a268821fa1
--- /dev/null
+++ b/mlir/test/Target/Cpp/unrealized_conversion_cast.mlir
@@ -0,0 +1,15 @@
+// RUN: mlir-translate -mlir-to-cpp %s | FileCheck %s
+
+// CHECK-LABEL: void builtin_cast
+func.func @builtin_cast(%arg0: !emitc.ptr<f32>){
+    // CHECK : float (*v2)[1][3][4][4] = (float(*)[1][3][4][4])v1
+  %1 = builtin.unrealized_conversion_cast %arg0 : !emitc.ptr<f32> to !emitc.array<1x3x4x4xf32>
+return
+}
+
+// CHECK-LABEL: void builtin_cast_index
+func.func @builtin_cast_index(%arg0:  !emitc.size_t){
+    // CHECK : size_t v2 = (size_t)v1
+  %1 = builtin.unrealized_conversion_cast %arg0 : !emitc.size_t to index
+return
+}
\ No newline at end of file

@LekkalaSravya3 LekkalaSravya3 changed the title Handled UnrealizedConversionCast for C code generation and validated … Handled UnrealizedConversionCast for C code generation and validated tests Sep 22, 2025
@LekkalaSravya3
Copy link
Author

LekkalaSravya3 commented Sep 24, 2025

@marbre , @ilovepi — when you have some time, could you please review this PR?

Copy link
Member

@marbre marbre left a comment

Choose a reason for hiding this comment

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

Drive-by comment. Also assigned reviewers, will try to allocate time for a proper review later if the others don't get to me before I have a chance.

Copy link
Contributor

@ilovepi ilovepi left a comment

Choose a reason for hiding this comment

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

LGTM.

@LekkalaSravya3 LekkalaSravya3 force-pushed the cpp-emitter_unrealized_conversion_cast branch from 354a234 to ec47d01 Compare September 25, 2025 10:19
@aniragil
Copy link
Contributor

Thanks for reporting and addressing this issue.
I'm not entirely sure though that the solution should be adding support for unrealized casts in the translator. As we end up with the unrealized type conversion, it seems our type conversion in MemRefToEmitC may not cover this case.
I wonder if the solution you propose (generating a cast to a pointer to an array and supporting it in emitc.subscript) can be implemented as part of lowering so that we don't get the unrealized cast in the first place?

@simon-camp
Copy link
Contributor

Hi @LekkalaSravya3, thanks for the contribution. Wouldn't we need to decay the outer most array dimension to pointer, i.e. array<2x3xi32> -> ptr<array<3xi32>>?
Additionally I also think this should be fixed in the lowering of memref.alloc and the type conversion. Though I haven't thought about all details.

Thanks for reporting and addressing this issue. I'm not entirely sure though that the solution should be adding support for unrealized casts in the translator. As we end up with the unrealized type conversion, it seems our type conversion in MemRefToEmitC may not cover this case. I wonder if the solution you propose (generating a cast to a pointer to an array and supporting it in emitc.subscript) can be implemented as part of lowering so that we don't get the unrealized cast in the first place?

I think that is the correct place to fix this. Adding support for ptr(array) is mostly involving updating the emitter. IIRC this type cannot be simply emitted left to right. so we would need a bit of book keeping in emitType. I think I have some python prototype lying around when/where to add parentheses into the emitted type.

Do you suggest that subscript will support emitc.subscript %a[%c3, %c1] : ptr<array<2xi32>> -> i32 directly? That vastly simplifies the problem then I think.
If we would need to split between pointer and array indexing it will get complicated, as the outer subscript won't be able to get materialized (it's emitted as an assignment to array type). Otherwise we would need to wrap all this into an expression with a combination of subscripts and loads.

@marbre marbre requested a review from simon-camp September 29, 2025 11:54
@aniragil
Copy link
Contributor

aniragil commented Oct 5, 2025

I think that is the correct place to fix this. Adding support for ptr(array) is mostly involving updating the emitter. IIRC this type cannot be simply emitted left to right. so we would need a bit of book keeping in emitType. I think I have some python prototype lying around when/where to add parentheses into the emitted type.

Not sure I follow: operators * and [] have the same (top) precedence and left-to-right associativity, right?

Do you suggest that subscript will support emitc.subscript %a[%c3, %c1] : ptr<array<2xi32>> -> i32 directly? That vastly simplifies the problem then I think.

Yes, as emitc.subscript already supports pointers but forces a single index in verify() we could arguably generalize it to support (element-type-rank + 1) indices.

If we would need to split between pointer and array indexing it will get complicated, as the outer subscript won't be able to get materialized (it's emitted as an assignment to array type).

Right, the emitc.apply * op predates the lvalue changes it doesn't return an lvalue but also loads the value (and is therefore marked as having a side effect). Since the pointer de-referencing would be done first, yielding an array, we'll need to add the emitc.apply op to the "deferred emission" mechanism when the de-referenced type is an array to solve the materialization problem and mark the op as not having a side effect in that case.

Otherwise we would need to wrap all this into an expression with a combination of subscripts and loads.

Right, as an alternative to adding emitc.apply to "deferred emission" (by load you mean the user of the subscript, right?).
The emitc.apply * side effect may still need to be refined so that the expression can be inline.

@LekkalaSravya3
Copy link
Author

Thank you for the detailed suggestion @simon-camp , @aniragil

From my understanding, I should modify the conversion to directly lower the input memref type to the !emitc.ptr<!emitc.array<...>> format and handle conversions using emitc::ApplyOp, ensuring that emitc::SubscriptOp supports the necessary index handling.

I just wanted to clarify one point — for cases like:

builtin.unrealized_conversion_cast %arg0 : !emitc.size_t to index

Would it make sense to handle the !emitc.size_t ↔ index conversion in a similar way within the TypeConverter? Specifically, by mapping index directly to !emitc.size_t and handling it uniformly during lowering, so we can avoid introducing UnrealizedConversionCastOp for these cases?

Would that approach be consistent with your suggestion?

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.

6 participants