-
Notifications
You must be signed in to change notification settings - Fork 146
[CIR][Lowering] Core dialects: passthrough symbol visibility #1620
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
Conversation
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.
Awesome, thanks for your first contributions. Can you please a testcase?
…to created func.func in cirtomlir lowering
… when convertion MLIR core module to LLVM MLIR module + added removal of unnecessary CIR Attr
adee41c
to
280bb46
Compare
Hey, I’ve added two test cases that are very similar to the examples I mentioned in the issue description. As expected, the tests failed without the changes and passed with them. This is my first time writing tests with lit/FileCheck, so if you notice anything that could be improved, I’d appreciate your feedback — but they might already be fine as they are. I was also unsure about the naming, so if you have better suggestions, feel free to let me know. |
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.
No problemo, here to help. Thanks for adding the tests - some nits but LGTM once fixed.
…onstraint to compilation test to fix execution on windows and mac
Thanks for the feedback! I've implemented the requested changes. Additionally, I added the -triple x86_64-unknown-linux-gnu option to the %clang_cc1 invocation in the declaration.c test, following the pattern used in other ThroughMLIR tests. I also constrained the compilation test to run only on x86_64 Linux targets, so hopefully both will now pass on Windows and macOS as well. I wasn't entirely sure which other targets to include. Based on CIRGenModule::getTargetCIRGenInfo(), it might make sense to relax the constraints in the future to support certain aarch64 targets (and possibly spirv64, nvptx, and amdgcn). But for now, I’ve conservatively limited the test to x86_64 Linux only. |
(and btw @bcardosolopes thanks for all the help and info on this PR and sorry about missing your suggestion about the space identation earlier) |
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.
Thanks for your patience, one last round of nits and LGTM!
Fixes #1619
Fixes #1618