[MLIR][EmitC] Add support for pointer-array types in the TypeConverter and related MemRef-to-EmitC operations, and update the C emitter. #160159
Conversation
|
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 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. |
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-emitc Author: Lekkala_Sravya-mcw (LekkalaSravya3) ChangesThis PR addresses issue #159746
Full diff: https://github.com/llvm/llvm-project/pull/160159.diff 2 Files Affected:
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
changed the title
marbre
left a comment
marbre
left a comment
There was a problem hiding this comment.
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.
LekkalaSravya3
force-pushed
the
cpp-emitter_unrealized_conversion_cast
branch
from
354a234 to
ec47d01
Compare
|
Thanks for reporting and addressing this issue. |
|
Hi @LekkalaSravya3, thanks for the contribution. Wouldn't we need to decay the outer most array dimension to pointer, i.e.
I think that is the correct place to fix this. Adding support for Do you suggest that subscript will support |
Not sure I follow: operators
Yes, as
Right, the
Right, as an alternative to adding |
|
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:
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? |
Right, except we were hoping you could use only
@simon-camp is probably more qualified than me to answer concretely, but in general I think the answer is yes - we should avoid generating unrealized casts where possible (and AFAIK when they do get created they are usually created in pairs such that they cancel each other later). Is that related to the same problem you're fixing in this PR? If not, best to do that in a separate PR. |
|
I would try to get this running https://godbolt.org/z/b7Ko1a6dv, by replacing But as I think about this now, you would get another unrealized conversion cast from |
|
@simon-camp Yes, when I converted it to |
|
Yes I think so. We need to be careful with allocas and globals though (and maybe function signatures, not sure), they need to be kept as Arrays so that the memory is allocated. Alloca can be converted to an Array variable and a Cast that decays the outer Dimension. |
…tests Signed-off-by: LekkalaSravya3 <[email protected]>
Signed-off-by: LekkalaSravya3 <[email protected]>
…ad/store and C-emitter Signed-off-by: LekkalaSravya3 <[email protected]>
LekkalaSravya3
force-pushed
the
cpp-emitter_unrealized_conversion_cast
branch
from
eff40b7 to
a76c5a4
Compare
LekkalaSravya3
changed the title
…t files Signed-off-by: LekkalaSravya3 <[email protected]>
LekkalaSravya3
force-pushed
the
cpp-emitter_unrealized_conversion_cast
branch
from
63812c9 to
23acdce
Compare
|
Hi @simon-camp , |
|
Hi @simon-camp — just checking in on this .. Before I proceed further with updates on the PR, could you please clarify the intended scope? |
|
I don't have a strong opinion on this, this can be revisited later. I had a look on your changes and it seems to me that you make the emission of the apply op deferred? Doesn't this change semantics of the apply op and affects downstream users? @aniragil what's your opinion on this. (This currently also conflicts with #159975) |
That's actually in the spirit of what I proposed. I was thinking this would only apply when the resulting type is an array so semantics wouldn't change, but this still makes the The "*" op will need to be introduced as I'm not sure we should introduce these ops as part of this patch, though. If everyone is OK on adding these ops I can update my old patch to use |
7 participants
This PR addresses issue #159746
This patch extends the MemRef-To-EmitC TypeConverter to handle pointer types, enabling proper conversion of MemRef types that are represented as ptr<array<>> .
The update ensures that operations involving allocations or memory access are correctly emitted in C as array-based pointer.
Key Changes Included :
The example file demonstrates how MemRef types using pointer are now correctly emitted as C code, addressing the issue described in #159746.
Example.txt