This is an archive of the discontinued LLVM Phabricator instance.

Break up Targets.cpp into a header/impl pair per target type[NFCI]
ClosedPublic

Authored by erichkeane on Jul 20 2017, 2:10 PM.

Details

Summary

Targets.cpp is getting unwieldy, and even minor changes cause the entire thing to cause recompilation for everyone. This patch bites the bullet and breaks it up into a number of files.

I tended to keep function definitions in the class declaration unless it caused additional includes to be necessary. In those cases, I pulled it over into the .cpp file. Content is copy/paste for the most part, besides includes/format/etc.

Diff Detail

Event Timeline

erichkeane created this revision.Jul 20 2017, 2:10 PM

I tended to keep function definitions in the class declaration unless it caused additional includes to be necessary.

Was that for implementation simplicity, or part of some cunning design goal? A lot of these pairs look pretty header-heavy to me (especially given that we're overriding virtual functions so inlining is mostly impossible).

Bitrig has to be eliminated - it's dead upstream and will be removed from LLVM. (confirmed from ex developers, nobody needs it any more)

I tended to keep function definitions in the class declaration unless it caused additional includes to be necessary.

Was that for implementation simplicity, or part of some cunning design goal? A lot of these pairs look pretty header-heavy to me (especially given that we're overriding virtual functions so inlining is mostly impossible).

No cunning design goal. Mostly just trying to keep the implementation time as low as possible, so that I don't get hit by a need to manually rebase next time someone makes a change here.

Bitrig has to be eliminated - it's dead upstream and will be removed from LLVM. (confirmed from ex developers, nobody needs it any more)

If no one has an objection, I can definitely remove that OS target and all references to it. Is that your encouragement here?

Bitrig has to be eliminated - it's dead upstream and will be removed from LLVM. (confirmed from ex developers, nobody needs it any more)

I just did a bit of looking about, and noticed that the "Bitrig" token is in a bunch of places. I suspect what we need is for a single pair of patches (llvm & clang) dedicated to removing it. I'd prefer to do those in a dedicated patchset if thats OK.

Bitrig has to be eliminated - it's dead upstream and will be removed from LLVM. (confirmed from ex developers, nobody needs it any more)

I just did a bit of looking about, and noticed that the "Bitrig" token is in a bunch of places. I suspect what we need is for a single pair of patches (llvm & clang) dedicated to removing it. I'd prefer to do those in a dedicated patchset if thats OK.

Please go for it.

Bitrig code has been merged back to OpenBSD.

Bitrig has to be eliminated - it's dead upstream and will be removed from LLVM. (confirmed from ex developers, nobody needs it any more)

I just did a bit of looking about, and noticed that the "Bitrig" token is in a bunch of places. I suspect what we need is for a single pair of patches (llvm & clang) dedicated to removing it. I'd prefer to do those in a dedicated patchset if thats OK.

Please go for it.

Bitrig code has been merged back to OpenBSD.

Ok, great! I'm working on a series of patches to delete it right now on top of this patch. I'll make sure to add you to the review.

craig.topper edited edge metadata.Jul 20 2017, 4:40 PM

Just review blank lines between every function. I'm too lazy to keep marking them.

lib/Basic/Targets/AArch64.cpp
21

Blank line

lib/Basic/Targets/AArch64.h
2

Header says x86

2

Also I think .h files are supposed to have 'cpp' in their top line.

17

Prevailing style is blank line between the include guard and the includes i think

20

Blank line

21

blank line

427

Blank line

469

Blank line

lib/Basic/Targets/AMDGPU.cpp
26

blank line

86

Blank line

93

Can we line break less often here.

546

blank line

567

blank line

lib/Basic/Targets/AMDGPU.h
24

Blank line

Changes for Craig

I think we should drop "using namespace llvm;" from the cpp files. clang doesn't usually do that except in codegen and it doesn't look like it was required in the original Targets.cpp.

craig.topper added inline comments.Jul 21 2017, 10:26 AM
lib/Basic/Targets/OSTargets.h
14

This comment seems somewhat redundant with the earlier one.

erichkeane added inline comments.Jul 21 2017, 10:29 AM
lib/Basic/Targets/OSTargets.h
14

Ah, yeah. That one came from copy/paste from the targets file. removed.

removed namespace llvm. was required in 1 place, but added namespace there instead.

erichkeane marked 16 inline comments as done.Jul 21 2017, 12:09 PM
echristo accepted this revision.Jul 21 2017, 1:29 PM

I'm going to say this ahead of time without looking into it "LGTM", but wait for ctopper (or someone else) to ack it for style etc since I'm unlikely to get to it any time shortly. :)

Also, make sure to clang format all the files please. I'm assuming you have, but...

-eric

This revision is now accepted and ready to land.Jul 21 2017, 1:29 PM

I'm going to say this ahead of time without looking into it "LGTM", but wait for ctopper (or someone else) to ack it for style etc since I'm unlikely to get to it any time shortly. :)

Also, make sure to clang format all the files please. I'm assuming you have, but...

-eric

Thanks! I did 'clang-format' in advance, though I reverted 1 version of it (the big list it turned into single entries for some reason). I'll wait for @craig.topper's finished.

Please recheck all the cpp file headers. Many of them aren't 80 columns and they should probably mention the target the way the header comments do. Right now it looks like a copy and paste from Targets.cpp with only the file name changed.

Clang format seems to have been really inconsistent about whether the closing curly brace for a static array is on a line by itself or the end of the previous line.

lib/Basic/Targets/AMDGPU.cpp
2

I think this line is short.

145

Closing curly on separate line.

lib/Basic/Targets/ARM.cpp
2

Short

55

Place losing curly on separate line

lib/Basic/Targets/AVR.h
79

Closing curly on separate line

lib/Basic/Targets/BPF.cpp
2

Line is short and description should match the header.

lib/Basic/Targets/Hexagon.cpp
113

Closing curly on separate line.

lib/Basic/Targets/MSP430.cpp
23

Closing curly on separate line

lib/Basic/Targets/Mips.cpp
2

This line looks short of 80 columns.

lib/Basic/Targets/Mips.h
219

Closing curly on separate line

364

Closing curly

376

Closing curly

lib/Basic/Targets/NVPTX.cpp
2

Short of 80 columns

lib/Basic/Targets/Nios2.cpp
2

Line is short and should probably match the header description.

lib/Basic/Targets/Nios2.h
105

Closing curly

lib/Basic/Targets/PNaCl.cpp
2

Short and probably the wrong description.

lib/Basic/Targets/PPC.cpp
450

Closing curly

lib/Basic/Targets/Sparc.cpp
2

Line is short

26

Closing curly

lib/Basic/Targets/SystemZ.cpp
36

Closing curly

lib/Basic/Targets/TCE.cpp
2

Line is short

lib/Basic/Targets/XCore.cpp
2

Line is short

erichkeane marked 20 inline comments as done.

Fixed all headers for name and length, and did all curly bracket changes suggested.

A few more curly brace fixups.

This revision was automatically updated to reflect the committed changes.
lib/Basic/Targets.h