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

Enhance the extensibility of TargetCIRGenInfo to support more targets #601

Open
seven-mile opened this issue May 13, 2024 · 1 comment
Open

Comments

@seven-mile
Copy link
Collaborator

The lib/CIR/CodeGen/TargetInfo.cpp collectively includes the implementations of TargetInfo and ABIInfo for the both targets we currently support: x86_64 and AArch64. Soon I may try merging the SPIR-V target support for CIRGen, so a refactor of this source file seems reasonable.

This also makes the structure matches the Clang CodeGen. Proposed structure:

  • lib/CIR/CodeGen/TargetInfo.cpp only includes TargetCIRGenInfo and a dispatcher CIRGenModule::getTargetCIRGenInfo
  • lib/CIR/CodeGen/ABIInfo.cpp only includes cir::ABIInfo
  • lib/CIR/CodeGen/Targets/XXX.cpp implements several target factories with the following signature:
std::unique_ptr<TargetCIRGenInfo> createXXXTargetCIRGenInfo(CIRGenModule &CGM);

Discussion: do we need an implementation of DefaultTargetCIRGenInfo and DefaultABIInfo like clang did? The ABIInfo of SPIR-V indeed has a dependency on DefaultABIInfo. We shall make it another patch anyway.

@seven-mile seven-mile changed the title Make TargetCIRGenInfo extensive to support more targets Enhance the extensibility of TargetCIRGenInfo to support more targets May 13, 2024
@bcardosolopes
Copy link
Member

@seven-mile thanks for opening this issue.

so a refactor of this source file seems reasonable.

+1

Discussion: do we need an implementation of DefaultTargetCIRGenInfo and DefaultABIInfo?

Sounds reasonable to me, we should have the skeleton as close as possible as to allow folks to make easier progress whenever adding things for new targets.

I think you should go ahead and make this refactoring, so you are not blocked. However, let me give you some remarks about some upcoming changes needed - in case that helps you somehow.

Part of the ABI information needs also to be available for LoweringPrepare and potentially other passes in the future and right now LoweringPrepare (lib/CIR/Transforms) doesn't have access to that information because it's in a sibling lib (lib/CIR/CodeGen) and doesn't really depend on it (see PR for vaargs and dynamic cast)

I'm a bit afraid that after you do this refactoring, we'd need to redesign some of it again; it's probably unfortunate to have to do this work twice. My rough sketch of the options are:

  1. Move this information to a new place so it's reusable by both lib/CIR/Transforms and lib/CIR/CodeGen. The challenges here are that some of this ABI bits are currently coupled with things like CGM or CGF, and we'd have to break them.
  2. Move lib/CIR/Transforms under lib/CIR/CodeGen/Transforms, this would still probably require breaking dep on CGM or CGF, but will likely make usual CIRGen stuff more accessible to transformations as well.

@sitio-couto is also gonna touch a lot of this soon. Can both of you chat/discuss/sync about an strategy that might lead to both of you taking advantage of each other's work?

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

No branches or pull requests

2 participants