-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add pass to emit helper funcs so that we have common call signature #1600
base: main
Are you sure you want to change the base?
Conversation
@@ -3,3 +3,4 @@ add_subdirectory(TTIR) | |||
add_subdirectory(TTNN) | |||
add_subdirectory(TTMetal) | |||
add_subdirectory(TTKernel) | |||
add_subdirectory(LLVM) |
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.
Note: imo this directory structure is most sensible. It's a little ugly to add a dir for "Dialect/LLVM" because LLVM isn't a new Dialect, and we only want to define a single Transform which is (softly) dependent on other TTIR passes to create module + funcs with expected attrs. On the other hand, jamming this Transform into TTIR or some other existing dir feels even worse to me, because this is strictly an LLVMDialect -> LLVMDialect Transform
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.
Yeah I'm not sure what's conventional here, @sdjordjevicTT, was anything like this covered at MLIR conf?
I generally agree with you though could see a loose argument for putting it in TTIR since it's defining the TTIR->CPU calling convention.
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 am honestly not sure, we didn't come up with something similar during the conference. I was observing a few mlir repos, and none of them had LLVM-specific folders in them, however not sure if any of them had something similar to implement.
Maybe the best advice we can get on this topic is their discord channel. @vwellsTT you can ask them this specific question here:
https://discord.com/channels/636084430946959380/642426447167881246
They are mostly responsive and helpful :)
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.
Ok--the discord link doesn't seem to take me to a channel properly though for some reason. Can you give me name or something?
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.
Ah, nvm, needed to join LLVM group first, now it works I think
608634e
to
9a6d461
Compare
mlir::tensor::TensorDialect, mlir::linalg::LinalgDialect, | ||
mlir::scf::SCFDialect, mlir::cf::ControlFlowDialect, | ||
mlir::tosa::TosaDialect, mlir::vector::VectorDialect, | ||
mlir::emitc::EmitCDialect, mlir::bufferization::BufferizationDialect, |
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 line is also modified by earlier PR, leading to some ugliness, but I think strictly speaking I only want to add mlir::LLVM::LLVMDialect for this PR to make testing easier, and earlier PRs will add Bufferization etc when merged
Perhaps adding my dylib shim at this stage would be useful for testing. So far, I've tested manually that the transform will run + produce legal output (w.r.t. to types, number of args, etc.) and that seems fine, but wouldn't be surprised at all if there's some logical bug in argument unpacking here |
7f71e72
to
2ca4886
Compare
7ea7691
to
6348c92
Compare
6348c92
to
b3e7289
Compare
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.
Things look good, but let's hold off on landing until we get other opinions on the directory structure.
@@ -3,3 +3,4 @@ add_subdirectory(TTIR) | |||
add_subdirectory(TTNN) | |||
add_subdirectory(TTMetal) | |||
add_subdirectory(TTKernel) | |||
add_subdirectory(LLVM) |
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.
Yeah I'm not sure what's conventional here, @sdjordjevicTT, was anything like this covered at MLIR conf?
I generally agree with you though could see a loose argument for putting it in TTIR since it's defining the TTIR->CPU calling convention.
Goal: The end-to-end goal is to integrate a path to compile and execute specific ops or sets of ops on the CPU.
Context:
The entire task will be split into (tentatively) 7 PRs, as follows:
This PR represents the 5th subtask above. The goal here is to make it so we can have a single entry point to call all of our dylib functions from the runtime side--to that end, we need helper funcs for each of our LLVM-lowered funcs which takes a generic pointer to a
tensor_wrapper
, and unpacks args (including variable size + stride args based on tensor rank). We accomplish this via annotating our lowered functions with tensor input ranks, and using that attr to infer number of sizes + strides to unpack, and number of input tensors total as well.Example
Input:
Output: