Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

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

Authored by barannikov88 on Apr 12 2023, 2:28 AM.

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

There are a very large number of changes, so older changes are hidden. Show Older Changes

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.