This is an archive of the discontinued LLVM Phabricator instance.

[clang][CodeGen] Break up TargetInfo.cpp [8/8]
ClosedPublic

Authored by barannikov88 on Apr 12 2023, 2:28 AM.
Tokens
"Baby Tequila" token, awarded by barannikov88.

Details

Summary

This commit breaks up CodeGen/TargetInfo.cpp into a set of *.cpp files,
one file per target. There are no functional changes, mostly just code moving.

Non-code-moving changes are:

  • A virtual destructor has been added to DefaultABIInfo to pin the vtable to a cpp file.
  • A few methods of ABIInfo and DefaultABIInfo were split into declaration + definition in order to reduce the number of transitive includes.
  • Several functions that used to be static have been placed in clang::CodeGen namespace so that they can be accessed from other cpp files.

RFC: https://discourse.llvm.org/t/rfc-splitting-clangs-targetinfo-cpp/69883

Diff Detail

Event Timeline

barannikov88 created this revision.Apr 12 2023, 2:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2023, 2:28 AM
barannikov88 retitled this revision from [clang][CodeGen] Break up TargetInfo.cpp [6/6] to [DRAFT][clang][CodeGen] Break up TargetInfo.cpp [6/6].Apr 12 2023, 3:06 AM
barannikov88 published this revision for review.Apr 12 2023, 3:52 AM
barannikov88 edited the summary of this revision. (Show Details)Apr 12 2023, 3:55 AM

ping
Does the approach look right?
Would it be better to put everything into cpp files and only expose a factory method e.g. createMyTargetCodeGenInfo?

I think this is a reasonable refactoring, but I'm hoping the CodeGen code owners can chime in with their thoughts as well given how large the changes are in their area. (I did not verify that the refactoring didn't change functionality, but spot-checking did not find any differences.)

This refactoring looks reasonable to me as well. In clang/lib/Driver, we have D30372 that splits some huge files into target-specific files.

asb added a comment.Apr 26 2023, 7:32 AM

+1 on this refactoring being a good idea. The RISC-V changes seem fine to me (haven't done a detailed line by line review).

myhsu added a subscriber: myhsu.Apr 26 2023, 12:54 PM

ping
Does the approach look right?

This definitely improves the readability and potentially the compilation time.

Would it be better to put everything into cpp files and only expose a factory method e.g. createMyTargetCodeGenInfo?

I'm fine with either approach. Also, the 68k part looks good.

Restructuring seems overall reasonable. (I'm assuming there isn't actually any changed code, just moving code around.)

Thank you all for taking a look!

I did not verify that the refactoring didn't change functionality, but spot-checking did not find any differences.

I'm assuming there isn't actually any changed code, just moving code around.

Apart from code moving this patch splits some methods into
declaration + definition so that the header files only need to include "TargetInfo.h".

This could be avoided by taking a different approach, i.e. the header files
contain only a factory function and cpp files contain the implementation
(of both ABIInfo and TargetCodeGenInfo). This is not a very C++-ish way,
but it would make the changes more straingforward.
I can do it if it's preferable.

This refactoring looks reasonable to me as well. In clang/lib/Driver, we have D30372 that splits some huge files into target-specific files.

Thanks, I've updated the RFC with this link.

  • Update references to modified files in non-code files
  • Add LLVM_LIBRARY_VISIBILITY to classes in header files
Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2023, 7:36 PM
  • Rebase on top of the stack
  • Fix a header comment
  • Split some virtual functions to avoid extra includes in header files
  • Forward declare a few classes and add necessary includes (IWYU, header files only)

Move interface comments to the header file

barannikov88 retitled this revision from [DRAFT][clang][CodeGen] Break up TargetInfo.cpp [6/6] to [clang][CodeGen] Break up TargetInfo.cpp [6/6].May 5 2023, 8:32 PM
barannikov88 edited the summary of this revision. (Show Details)
barannikov88 edited the summary of this revision. (Show Details)May 5 2023, 8:39 PM

I think it should be ready for review now.

One point to note.
I didn't follow IWYU rule in target *.cpp files -- they include the smallest set
of headers for now. My memory suggests me that IWYU is a requirement,
but I couldn't find a proof in the coding standard. It now says
"include as little as possible" and further elaborates:

You must include all of the header files that you are using — you can include
them either directly or indirectly through another header file.

I also think it is easier to fix compiler errors afterwards than to try to figure out
the necessary or reasonable set of includes. Please let me know if you disagree.

barannikov88 edited the summary of this revision. (Show Details)
  • Forward declare CodeGenTypes
  • Split a few more virtual functions into declaration+definition to not depend on transitive includes, which are going to be removed
barannikov88 updated this revision to Diff 520041.EditedMay 6 2023, 1:18 AM
  • Move misplaced isHomogenousAggregate definition
  • Rearrange definitions in ABIInfo.cpp to match the order of declarations
  • Drop some includes in TargetInfo.h that iwyu utility classified as completely unused
MaskRay added inline comments.May 6 2023, 2:22 PM
clang/lib/CodeGen/ABIInfo.h
92

Remove "isHomogeneousAggregate - "

https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments
"Don’t duplicate function or class name at the beginning of the comment."

barannikov88 retitled this revision from [clang][CodeGen] Break up TargetInfo.cpp [6/6] to [clang][CodeGen] Break up TargetInfo.cpp [7/7].
barannikov88 edited the summary of this revision. (Show Details)

Rebase

Remove introduced duplicate functions

barannikov88 retitled this revision from [clang][CodeGen] Break up TargetInfo.cpp [7/7] to [clang][CodeGen] Break up TargetInfo.cpp [8/8].
barannikov88 edited the summary of this revision. (Show Details)

Rebase on top of the commit that introduced factory functions

Squash commits

Undo file renaming so that it does not look like the file was replaced

Undo unintended formatting changes

Undo reformatting in arch-specific files
This should help in resolving merge conflicts downstream

ping :)
Any comments / questions / suggestions are welcome

Rebase & ping

Move complexTempStructure to its only user (PPC.cpp)

Add a virtual destructor to DefaultABIInfo to pin the vtable to a cpp file

Update BUILD.gn

barannikov88 edited the summary of this revision. (Show Details)May 17 2023, 7:02 PM
barannikov88 edited the summary of this revision. (Show Details)May 17 2023, 7:11 PM

Undo remaining unintended formatting changes

This revision is now accepted and ready to land.Jun 5 2023, 11:27 AM

@efriedma Thank you for all your reviews!

barannikov88 edited the summary of this revision. (Show Details)Jun 16 2023, 8:13 PM
This revision was landed with ongoing or failed builds.Jun 16 2023, 9:15 PM
This revision was automatically updated to reflect the committed changes.
clang/lib/CodeGen/Targets/WebAssembly.cpp