-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[mlir][EmitC] Expand the MemRefToEmitC pass - Lowering reinterpret_cast
#152610
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
base: main
Are you sure you want to change the base?
Conversation
In your example, |
emitc::ConstantOp zeroIndex = rewriter.create<emitc::ConstantOp>( | ||
loc, rewriter.getIndexType(), rewriter.getIndexAttr(0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've seen a zeroIndex crated in several places now. Its not much code, but that may be a good candidate for a helper function (e.g. in the anonymous namespace and marked static).
llvm::SmallVector<mlir::Value> indices; | ||
for (int i = 0; i < rank; ++i) { | ||
indices.push_back(zeroIndex); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
llvm::SmallVector<mlir::Value> indices; | |
for (int i = 0; i < rank; ++i) { | |
indices.push_back(zeroIndex); | |
} | |
llvm::SmallVector<mlir::Value> indices(rank, zeroIndex); |
auto createPointerFromEmitcArray = | ||
[loc, &rewriter, &zeroIndex]( | ||
mlir::TypedValue<emitc::ArrayType> arrayValue) -> emitc::ApplyOp { | ||
int64_t rank = arrayValue.getType().getRank(); | ||
llvm::SmallVector<mlir::Value> indices; | ||
for (int i = 0; i < rank; ++i) { | ||
indices.push_back(zeroIndex); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This closure seems familar from your other patches. I'm guessing that means it should probably be a helper function instead, since I don't see anything in the capture list that couldn't be a parameter.
//CHECK-NEXT: %1 = "emitc.constant"() <{value = 0 : index}> : () -> index | ||
//CHECK-NEXT: %2 = emitc.subscript %0[%1] : (!emitc.array<999xi32>, index) -> !emitc.lvalue<i32> | ||
//CHECK-NEXT: %3 = emitc.apply "&"(%2) : (!emitc.lvalue<i32>) -> !emitc.ptr<i32> | ||
//CHECK-NEXT: %4 = emitc.call_opaque "reinterpret_cast"(%3) {args = [0 : index], template_args = [!emitc.ptr<!emitc.array<1x1x999xi32>>]} : (!emitc.ptr<i32>) -> !emitc.ptr<!emitc.array<1x1x999xi32>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an actual reinterpret_cast<T>()
is appropriate for C++, but for C, you'd need a C-style cast. given that the C-style cast will work in both languages and what you're doing can't be covered by static or dyn casts, maybe we should use a normal cast operation? Would that work here, do you think?
This patch lowers
memref.reinterpret_cast
.From:
To: