Open seven-mile opened 1 month ago
@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:
@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?
The
lib/CIR/CodeGen/TargetInfo.cpp
collectively includes the implementations ofTargetInfo
andABIInfo
for the both targets we currently support:x86_64
andAArch64
. 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 includesTargetCIRGenInfo
and a dispatcherCIRGenModule::getTargetCIRGenInfo
lib/CIR/CodeGen/ABIInfo.cpp
only includescir::ABIInfo
lib/CIR/CodeGen/Targets/XXX.cpp
implements several target factories with the following signature:Discussion: do we need an implementation of
DefaultTargetCIRGenInfo
andDefaultABIInfo
like clang did? TheABIInfo
of SPIR-V indeed has a dependency onDefaultABIInfo
. We shall make it another patch anyway.