Details

Reviewers

rjmccall
aaron.ballman
erichkeane
jdoerfert
efriedma
DavidSpickett

Commits

rG992cb98462ab: [clang][CodeGen] Break up TargetInfo.cpp [8/8]

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

Repository: rG LLVM Github Monorepo

Event Timeline

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

In D148094#4284316, @barannikov88 wrote:

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.)

barannikov88 removed parent revisions: D148092: [clang][CodeGen] Break up TargetInfo.cpp [4/8], D148091: [clang][CodeGen] Break up TargetInfo.cpp [3/8], D148090: [clang][CodeGen] Break up TargetInfo.cpp [2/8], D148089: [clang][CodeGen] Break up TargetInfo.cpp [1/8].Apr 29 2023, 9:53 AM

Rebase

Harbormaster completed remote builds in B229036: Diff 518199.Apr 29 2023, 10:53 AM

Thank you all for taking a look!

In D148094#4296346, @aaron.ballman wrote:

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

In D148094#4302879, @efriedma wrote:

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.

In D148094#4296351, @MaskRay wrote:

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

Herald added a subscriber: llvm-commits. · View Herald Transcript

Harbormaster completed remote builds in B229093: Diff 518264.Apr 29 2023, 8:49 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)

Harbormaster completed remote builds in B230359: Diff 520015.May 5 2023, 7:34 PM

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.

Harbormaster completed remote builds in B230367: Diff 520025.May 5 2023, 10:03 PM

Forward declare CodeGenTypes
Split a few more virtual functions into declaration+definition to not depend on transitive includes, which are going to be removed

Harbormaster completed remote builds in B230375: Diff 520034.May 5 2023, 11:18 PM

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

Harbormaster completed remote builds in B230382: Diff 520041.May 6 2023, 2:38 AM

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."