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

How should we handle va_arg? #862

Open
ChuanqiXu9 opened this issue Sep 19, 2024 · 8 comments
Open

How should we handle va_arg? #862

ChuanqiXu9 opened this issue Sep 19, 2024 · 8 comments
Assignees

Comments

@ChuanqiXu9
Copy link
Member

ChuanqiXu9 commented Sep 19, 2024

I noticed both #573 and #95 mentioned that va_arg is not implemented due to it is ABI specific. But it looks like the Itanium C++ ABI (https://itanium-cxx-abi.github.io/cxx-abi/abi.html) didn't mention how to implement va_arg. Can't we lower it to https://llvm.org/docs/LangRef.html#va-arg-instruction directly?

CC: @bcardosolopes @ghehg @sitio-couto

@ChuanqiXu9
Copy link
Member Author

It looks like the direct reason is that, MLIR doesn't offer mlir::LLVM::VAArgOp in mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td while it offers mlir::LLVM::VaStartOp, mlir::LLVM::VaEndOp and mlir::LLVM::VaCopyOp. So if we want to support va_arg in CIR, we have to touch the different targets. Do I understand the problem correctly?

And if yes, it looks like we can't use llvm's va-arg-instruction directly due to some layering issues. I am wondering if we can make it by adding these layers in CIR. Since I don't feel it is right to handle these architecture specific things in CIR. It should be the job of LLVM.

@ChuanqiXu9
Copy link
Member Author

Sent #865

@bcardosolopes
Copy link
Member

bcardosolopes commented Sep 19, 2024

Since I don't feel it is right to handle these architecture specific things in CIR. It should be the job of LLVM.

To give some extra context: YMMV here, somethings are lowered early and some of them late in the pipeline, it depends on a number of factors. Example: out of CIRGen we have some layers for vtable access while for some types (like doubles) we impose their underlying ABI widths early. Calling convention lowering is a mid pipeline example: it's done CIR to CIR before LLVM lowering, since we don't want to reconstruct things if we are doing analysis that require some level of source fidelity. The decision is usually made considering potential (or more concrete) use cases, with a special care not to over design anything that we don't have a use case for.

For the vaarg in question, your direction sounds good, thanks for working on this. @sitio-couto since you have the most fresh context in this area, anything you'd like to comment?

@ghehg
Copy link
Collaborator

ghehg commented Sep 19, 2024

It looks like the direct reason is that, MLIR doesn't offer mlir::LLVM::VAArgOp in mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td while it offers mlir::LLVM::VaStartOp, mlir::LLVM::VaEndOp and mlir::LLVM::VaCopyOp. So if we want to support va_arg in CIR, we have to touch the different targets. Do I understand the problem correctly?

Yes, It was a reason I had to do ABI specific lowering in CIR Lowering Prepare for var_arg. Another reason is that clang codegen does ABI-specific lowering, and not lowering to llvm var_arg instruction, probably for a good reason.

RValue AArch64ABIInfo::EmitAAPCSVAArg(Address VAListAddr, QualType Ty,

There were discussions and concerns about the instruction in the past.
https://lists.llvm.org/pipermail/llvm-dev/2017-August/116337.html
https://discourse.llvm.org/t/va-arg-on-windows-64/40780

And if yes, it looks like we can't use llvm's va-arg-instruction directly due to some layering issues. I am wondering if we can make it by adding these layers in CIR. Since I don't feel it is right to handle these architecture specific things in CIR. It should be the job of LLVM.

In general, I agree with your direction. For this matter, better if CIR leaves the ABI specific implementation to LLVM backend. But that being said, we need to have trust that llvm var_arg instruction has reliable and good support for ABIs.
See current clang llvm is not using va_arg instruction
https://godbolt.org/z/1bTK11cMn

@bcardosolopes
Copy link
Member

Thanks for bringing this into attention @ghehg, I didn't recall that Clang wasn't really using this instruction, good catch.

we need to have trust that llvm var_arg instruction has reliable and good support for ABIs. See current clang llvm is not using va_arg instruction: https://godbolt.org/z/1bTK11cMn

+1, we need to lower to very similar code, summary of this discussions so far:

  • OG codegen lower early because ABI is hard, does not use var_arg.
  • LLVM code coming from CIR should be very similar to OG.
  • x86_64 support for var_arg should be implemented in target lowering, similar to like we do for aarch64. If it's similar to x86_64 perhaps it should be refactor to the default or whatever makes more sense.

@ChuanqiXu9 ChuanqiXu9 self-assigned this Sep 24, 2024
@ChuanqiXu9
Copy link
Member Author

Fine enough. I'll try to use the patch in the downstream first and go back to implement it as we like after I have a better understanding of the problem scope.

@bcardosolopes
Copy link
Member

FWIW, we don't need that operation in LLVM IR dialect for the sake of ClangIR lowering, since the desired approach has already been discusses here and in the PR.

@ChuanqiXu9
Copy link
Member Author

FWIW, we don't need that operation in LLVM IR dialect for the sake of ClangIR lowering, since the desired approach has already been discusses here and in the PR.

Practically, yet. But technically, the LLVM IR dialect is still required by some other targets.

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 a pull request may close this issue.

3 participants