Skip to content

[CIR][cir-translate] Support specifying target for cir-translate #1186

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Dec 12, 2024
Merged

[CIR][cir-translate] Support specifying target for cir-translate #1186

merged 7 commits into from
Dec 12, 2024

Conversation

seven-mile
Copy link
Collaborator

@seven-mile seven-mile commented Nov 28, 2024 •
edited
Loading

This PR adds a new command line option --target to our tool cir-translate. The concrete behaviour of it also depends on the triple and data layout in the CIR module. See the table in code comments for details.

The default triple is x86_64-unknown-linux-gnu currently.

Some tests are updated with triple and DLTI attribute eliminated (replaced by an option in RUN line). But still some tests remain unchanged, primarily because they use cir-opt instead.

bcardosolopes
bcardosolopes reviewed Dec 2, 2024
View reviewed changes
Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

Overall looks great, couple of questions

bcardosolopes
bcardosolopes requested changes Dec 3, 2024
View reviewed changes
@seven-mile
Copy link
Collaborator Author

Suggestions applied. The test files are not merged using -split-input-file, because they do not share the RUN line.

bcardosolopes
bcardosolopes approved these changes Dec 6, 2024
View reviewed changes
Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

I just noticed way more tests (maybe you added more? I might have missed), that said, probably best to land them all under test/CIR/Tools/cir-translate/<name-of-test>. Will merge once this is changed!

@bcardosolopes
Copy link
Member

no need for the target-option subdir, one level for the tool is good enough :)

@seven-mile
Copy link
Collaborator Author

Gentle ping

@keryell
Copy link
Collaborator

keryell commented Dec 11, 2024

Just curious: why is the triple necessary for generating LLVM IR with cir-translate but not MLIR LLVM dialect with cir-opt?

@seven-mile
Copy link
Collaborator Author

Just curious: why is the triple necessary for generating LLVM IR with cir-translate but not MLIR LLVM dialect with cir-opt?

@keryell Actually both are expected to have triple and data layout. cir-opt is just not supported yet, for many reasons.

@bcardosolopes
Copy link
Member

Sorry for the delay @seven-mile, taking random vacation days in December! Thanks for working on this

@bcardosolopes bcardosolopes merged commit 7eb09a7 into main Dec 12, 2024
6 checks passed
@bcardosolopes
Copy link
Member

Just curious: why is the triple necessary for generating LLVM IR with cir-translate but not MLIR LLVM dialect with cir-opt?

@keryell Actually both are expected to have triple and data layout. cir-opt is just not supported yet, for many reasons.

maybe we should create an issue to track it?

lanza pushed a commit that referenced this pull request Mar 18, 2025
@seven-mile @lanza
[CIR][cir-translate] Support specifying target for cir-translate (#…
d0253d9 
…1186)

This PR adds a new command line option `--target` to our tool
`cir-translate`. The concrete behaviour of it also depends on the triple
and data layout in the CIR module. See the table in code comments for
details.

The default triple is `x86_64-unknown-linux-gnu` currently.

Some tests are updated with triple and DLTI attribute eliminated
(replaced by an option in RUN line). But still some tests remain
unchanged, primarily because they use `cir-opt` instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bcardosolopes bcardosolopes bcardosolopes approved these changes

@lanza lanza Awaiting requested review from lanza lanza is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@seven-mile @bcardosolopes @keryell