Skip to content
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

Support lowering VAArgOp #865

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

ChuanqiXu9
Copy link
Member

Close #862

I made this by introducing VaArgOp to mlir and use that op in clangir. I feel this is the most natural way to do this. I don't think we should handle different architecture in clangir. It is the job of LLVM or MLIR. I understand that we hope we don't touch any other workdirs except CIR. But it looks pretty better to do the proper thing in the proper places.

I'll try to upstream the patch to support MLIR later.

@ChuanqiXu9
Copy link
Member Author

CC @ghehg @sitio-couto since I can't add reviewers.

@bcardosolopes
Copy link
Member

since I can't add reviewers.

Just fixed that.

@bcardosolopes
Copy link
Member

bcardosolopes commented Sep 19, 2024

I'll try to upstream the patch to support MLIR later.

Thanks for fixing this, we indeed need that op in the LLVM dialect to make progress. We are very afraid of the technical debt though, specially the ones outside CIR stuff. The LLVM part should be done upstream first and cherry-picked here. It should also be in a different PR from the actual lib/CIR changes (so our rebaser can make into an empty commit once it kicks in against upstream, cc @lanza).

EDIT: perhaps this won't be needed at all.

@ghehg
Copy link
Collaborator

ghehg commented Sep 19, 2024

Thanks for working on this. This is necessary along right direction. I agree with @bcardosolopes, it might be better if we have LLVM dialect part of change landed in upstream first, then other parts of this PR can just rebase on it, and go. This way we don't have to sync changes of upstream PR and that of this PR. Let me know what you think.

Copy link
Collaborator

@sitio-couto sitio-couto left a comment

Choose a reason for hiding this comment

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

Hey @ChuanqiXu9!

I really like this approach—it's clean and a great contribution to the LLVM Dialect.

I don't think we should handle different architecture in clangir.

AFAIK, we need to handle targets in CIR to some extent, since LLVM IR doesn’t cover all the details. Still, this is a good abstraction.

One concern I have is that, for some reason, the OG Clang doesn’t use the LLVM va_arg instruction when lowering C/C++.
Maybe it's just technical debt. Maybe it's something more complex, like optimizations (choosing between reg or mem like the example above).
By emitting a different IR than the original CodeGen, we might be abstracting something we shouldn’t, like an optimization or argument-passing correctness.

Just something to keep in mind, otherwise LGTM!

@bcardosolopes
Copy link
Member

One concern I have is that, for some reason, the OG Clang doesn’t use the LLVM va_arg instruction when lowering C/C++.

Yea, same feedback in the issue. The PR should work along the lines of Aarch64 here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How should we handle va_arg?
4 participants