Page MenuHomePhabricator

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

Repository
rL LLVM

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
20 ↗(On Diff #107581)

Blank line

lib/Basic/Targets/AArch64.h
1 ↗(On Diff #107581)

Header says x86

1 ↗(On Diff #107581)

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

16 ↗(On Diff #107581)

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

19 ↗(On Diff #107581)

Blank line

20 ↗(On Diff #107581)

blank line

426 ↗(On Diff #107581)

Blank line

468 ↗(On Diff #107581)

Blank line

lib/Basic/Targets/AMDGPU.cpp
25 ↗(On Diff #107581)

blank line

85 ↗(On Diff #107581)

Blank line

92 ↗(On Diff #107581)

Can we line break less often here.

545 ↗(On Diff #107581)

blank line

566 ↗(On Diff #107581)

blank line

lib/Basic/Targets/AMDGPU.h
23 ↗(On Diff #107581)

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
13 ↗(On Diff #107682)

This comment seems somewhat redundant with the earlier one.

erichkeane added inline comments.Jul 21 2017, 10:29 AM
lib/Basic/Targets/OSTargets.h
13 ↗(On Diff #107682)

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
1 ↗(On Diff #107693)

I think this line is short.

144 ↗(On Diff #107693)

Closing curly on separate line.

lib/Basic/Targets/ARM.cpp
1 ↗(On Diff #107693)

Short

54 ↗(On Diff #107693)

Place losing curly on separate line

lib/Basic/Targets/AVR.h
78 ↗(On Diff #107693)

Closing curly on separate line

lib/Basic/Targets/BPF.cpp
1 ↗(On Diff #107693)

Line is short and description should match the header.

lib/Basic/Targets/Hexagon.cpp
112 ↗(On Diff #107693)

Closing curly on separate line.

lib/Basic/Targets/MSP430.cpp
22 ↗(On Diff #107693)

Closing curly on separate line

lib/Basic/Targets/Mips.cpp
1 ↗(On Diff #107693)

This line looks short of 80 columns.

lib/Basic/Targets/Mips.h
218 ↗(On Diff #107693)

Closing curly on separate line

363 ↗(On Diff #107693)

Closing curly

375 ↗(On Diff #107693)

Closing curly

lib/Basic/Targets/NVPTX.cpp
1 ↗(On Diff #107693)

Short of 80 columns

lib/Basic/Targets/Nios2.cpp
1 ↗(On Diff #107693)

Line is short and should probably match the header description.

lib/Basic/Targets/Nios2.h
104 ↗(On Diff #107693)

Closing curly

lib/Basic/Targets/PNaCl.cpp
1 ↗(On Diff #107693)

Short and probably the wrong description.

lib/Basic/Targets/PPC.cpp
449 ↗(On Diff #107693)

Closing curly

lib/Basic/Targets/Sparc.cpp
1 ↗(On Diff #107693)

Line is short

25 ↗(On Diff #107693)

Closing curly

lib/Basic/Targets/SystemZ.cpp
35 ↗(On Diff #107693)

Closing curly

lib/Basic/Targets/TCE.cpp
1 ↗(On Diff #107693)

Line is short

lib/Basic/Targets/XCore.cpp
1 ↗(On Diff #107693)

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.