This is an archive of the discontinued LLVM Phabricator instance.

Propeller lld framework for basicblock sections
Needs ReviewPublic

Authored by shenhan on Sep 25 2019, 4:57 PM.

Details

Summary

Propeller lld framework for basicblock sections.

This is part of the Propeller framework to do post link code layout optimizations. Please see the RFC here: https://groups.google.com/forum/#!msg/llvm-dev/ef3mKzAdJ7U/1shV64BYBAAJ and the detailed RFC doc here: https://github.com/google/llvm-propeller/blob/plo-dev/Propeller_RFC.pdf

This is one in the series of patches for Propeller.

This patch adds the following to lld:

  1. Parses each object file and create CFG for each function.
  2. Applies propeller profiles to CFGs.
  3. Calls FunctionReordering/BBReodering and generates symbol ordering file. [this part is in a separate CL]

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

shenhan created this revision.Sep 25 2019, 4:57 PM

I haven't looked into the details yet, but here are some comments that are generally applicable to this patch.
After these are fixed, I may create some diffs for you to apply (they may be easier for me than adding comments here and there).

lld/ELF/Propeller.cpp
2

Add license notice.

126

Omit == true

178

Expand auto types unless they are obvious from the right hand side expression (e.g. make_unique)

339

Omit {} for simple statements.

lld/ELF/Propeller.h
2

Add license notice.
Use variableName instead of VariableName to conform to the local naming convention.

19

Delete it. Common llvm types are exported by include/lld/Common/LLVM.h which is transitively included in most headers.

21

Delete such using std::$container;

114

Use MemoryBuffer instead of FILE*.

Consider elf::readFile or

auto mbOrErr = MemoryBuffer::getFile(path, -1, false);
119

Prefer a vector to a malloc allocated buffer.

137

Prefer unique_ptr to raw pointers.

ruiu added a comment.Sep 25 2019, 8:35 PM

Let me start from very high-level review comments before getting into the details of the actual code:

  • First and foremost, can you add a file comments to each file to explain what you are doing in the file? It doesn't have to be detailed, but giving an overview at the beginning of a file is generally very useful.
  • As Michael pointed out in the RFC thread, we already have a similar feature (C3 ordering) in lld to order sections according to its graph structure. These two should probably be merged.

Some suggestions for comments are inlined.

lld/test/ELF/propeller/propeller-error-on-bblabels.s
104

(A comment that applies to all test files in this patch.)

Scrub unnecessary assembly instructions and directives. Unnecessary directives include .file, .ident, .addrsig, etc. .addrsig is only useful if you want to test propeller's interaction with --icf=safe. .file is only necessary if the filename is significant.

Please also make basic blocks as simple as possible. The tests take more than 3000 lines of code. They will be very difficult to change if someone proposes a new feature that somehow interacts with propeller and has to fix all the tests.

For example, for the instructions below, one or two is probably sufficient. It is entirely unnecessary to have tens of instructions that are not relocated and are not used in any CHECK lines.

movq  %rax, %rdx
shrq  $63, %rdx
sarq  $34, %rax
addl  %edx, %eax
addl  %eax, %eax

Some clang generated comments should also be removed, e.g.

movhpd -8(%rsp), %xmm0 # xmm0 = xmm0[0],mem[0]

When the test is long, besides the file-level comment ## , add some inline comments. Again, most people are unfamiliar with the feature and the comment will help them if they have to adjust propeller tests.

lld/test/ELF/propeller/propeller-keep-bb-symbols.s
2

Most -triple=x86_64-pc-linux can be -triple=x86_64. They are applicable to all ELF platforms, not just Linux.

4

I doubt $(...) (bash syntax) can be used in portable lit tests. There is no other 'RUN.*\$\(' usage. use %p (https://llvm.org/docs/CommandGuide/lit.html#pre-defined-substitutions)

9

Use ## for comments, # RUN: for RUN lines or CHECK lines.

10

[[ is bash-ism. They will fail if the test runner does not use bash. Prefer FileCheck to grep.

When you don't need the size field, don't add -S to the llvm-nm command line.

lld/test/ELF/propeller/propeller-opt-all-combinations.s
185

Indent instructions as you do for instructions above.

shenhan updated this revision to Diff 222052.Sep 26 2019, 4:57 PM
shenhan marked 9 inline comments as done.

Addressed maskray's comments.

shenhan marked 6 inline comments as done.Sep 26 2019, 4:59 PM

Let me start from very high-level review comments before getting into the details of the actual code:

  • First and foremost, can you add a file comments to each file to explain what you are doing in the file? It doesn't have to be detailed, but giving an overview at the beginning of a file is generally very useful.

Thanks, Done.

  • As Michael pointed out in the RFC thread, we already have a similar feature (C3 ordering) in lld to order sections according to its graph structure. These two should probably be merged.

Yup, that is reasonable. We discussed the issue this morning and think it is feasible to merge the C3 ordering w/ part of our reordering algorithm. However, we probably will delay this until after we get the first version in.

shenhan updated this revision to Diff 222064.Sep 26 2019, 5:52 PM
ruiu added inline comments.Sep 26 2019, 8:35 PM
lld/ELF/Propeller.h
10–14

This is too terse and can be understood only by those who knows what Propeller is/does. I want you to write something like this: https://github.com/llvm/llvm-project/blob/master/lld/ELF/ICF.cpp.

MaskRay added inline comments.Sep 27 2019, 12:32 AM
lld/ELF/Propeller.cpp
28

PropellerBBReordering.h is added in D68073, not in this patch. Please make each patch compilable.

52

Delete using std::$container.

lld/ELF/Propeller.h
2

Use variableName instead of VariableName to conform to the local naming convention.

This is not done.

243

std::unique_ptr
std::mutex

lld/ELF/PropellerELFCfg.h
157

Delete {}

This is applicable to many places of the patch series.

shenhan updated this revision to Diff 222270.Sep 27 2019, 5:21 PM
shenhan marked 2 inline comments as done.

Updated header file description. Also changed local variable names to lower case camel.

shenhan marked 2 inline comments as done.Sep 27 2019, 5:22 PM
shenhan added inline comments.
lld/ELF/Propeller.cpp
28

There are 3 patches in lld, and our plan is to submit all in one shot, so we marked patch relationships (parent/child) for all these 3 . (if phabricate does not support this operation, we'll submit it locally) Does this seem reasonable to you?

lld/ELF/Propeller.h
2

A little bit confused. https://llvm.org/docs/CodingStandards.html states that noun name should be camel cased, with first letter in upper case. However I also see LLD files have used "smallUpperCaseNames" as locals...

I'll conform to lld.

MaskRay added inline comments.Sep 27 2019, 8:22 PM
lld/ELF/Propeller.cpp
532

Delete adhoc timers. There is currently no such timer mechanism in lld, but if we do, we should use proper measuring mechanisms like llvm::TimeRegion, and have a user interface like gcc/clang -ftime-record, not ad-hoc stdout dumps.

lld/ELF/Propeller.h
2

LLD's variable naming convention was changed in D64121.

21

There are still lots of using std::string or using std::foobar here and there.

249

std::unique_ptr defaults to std::default_delete.

lld/ELF/PropellerELFCfg.h
64

The interface here, and the one in the hfsort BBreordering patch are overly complex. Some abstractions are not really needed. Delete friend declarations, make some classes struct, and migrate from std::list to more efficient containers. There should be a rethink how the graph theory abstractions are connected.

157

Still lots of {} surrounding simple statements.

239

Not clang-format'ed.

ruiu added inline comments.Sep 29 2019, 7:15 PM
lld/ELF/Propeller.cpp
28

Each individual commit to lld should compile. Otherwise, buildbots (or other human developers) could sync to a tree that doesn't compile, and that is inconvenient for them. It also makes it hard to debug by bisecting. So, if you are going to submit these patches in one shot, please make sure that you merge the patches into a single commit first and then commit.

ruiu added a comment.Sep 29 2019, 11:32 PM

I just skimmed through this patch, so please expect more review comments. Also as I don't understand the whole picture of this patch yet, I'm not certain if the design of this patch is right. But let me start with this first-round review.

lld/ELF/Propeller.h
10

Maybe this is a good place to explain what Propeller is in the first place. I'd start this comment with something like: Propeller is a profile-guided optimization implemented in lld, Clang and LLVM that reorders code and data so that the resulting program has better cache locality. Propeller works on top of ThinLTO and the regular PGO and improves performance by a few percent. Propeller can be thought as an extension to -ffunction-sections and -fdata-sections, that are compiler options to emit each function and a data item to a separate section. With Propeller, the granularity is not function but basic block; each basic block is emitted as a separate section. That allows the linker to reorder basic blocks. etc. etc...

18

Cfg -> CFG because it's an acronym. We have a Config object, and if you spell CFG as Cfg, it would look like a Config object.

21

CFGs

(but is plural correct?)

22

bb -> BB

58

Remove these using directives. It is not allowed to import names from std in the LLVM coding style.

73

Do you need these ELF prefixes even if they are in lld::elf namespace?

Cfg -> CFG

81

a -> A

84

Please reduce the number of lines of this example so that the example contains just enough information to explain your idea.

159–164

You shouldn't manage these resources by hand. Instead, use std::unique_ptr, raw_stream, etc. I think you can eliminate this hand-written dtor altogether.

207

You can use lld::bAlloc instead of defining your own.

217

std::map is generally slower than llvm::DenseMap. Do you need to use std::map?

219–222

Format

265–266

Mutex is used for locking, so this comment isn't useful. Please explain what resource you actually want to guard using this lock.

shenhan updated this revision to Diff 222524.Sep 30 2019, 5:26 PM
shenhan marked 36 inline comments as done and an inline comment as not done.
shenhan added inline comments.
lld/ELF/Propeller.cpp
52

Deleted all "std:*".

lld/ELF/Propeller.h
10

Thanks. I added a section "About propeller framework" at the top of Propeller.h.

10–14

Thanks. I pre-pended paragraphs w/ more details to this part.

18

Yes, changed single word Cfg -> CFG.

(But kept "Cfg" in "ELFCfg", "ELFCfgNode", "ELFCfgBuiilder", etc unchanged, cause Cfg in such a context shall keep the camel rule and is less prone to be mistaken for Config object"...)

21

Replaced CFGs w/ "control flow graphs".

21

Removed all "using std::*", and lots of "using llvm::*".

73

Actually these are in lld::propeller namespace.

137

unique_ptr<char> type does not work smoothly w/ "getline(char **lineptr, size_t *n, FILE *stream)", which accepts a pointer to pointer parameter, which could be deallocated / reallocated by getline.

lld/ELF/PropellerELFCfg.h
157

BTW, I intentionally keep "{}" in these scenarios, if that makes sense to you.

for (...) {

if (...) {
  for (...)
    xxx
} else {
  yyy
}

}

shenhan added inline comments.Sep 30 2019, 5:27 PM
lld/ELF/Propeller.cpp
28

Thanks. Yes, that makes sense.

28

We will merge 3 patches into 1 and submit in one shot.

lld/ELF/Propeller.h
159–164

unique_ptr<char> type does not work smoothly w/ "getline(char **lineptr, size_t *n, FILE *stream)", which accepts a pointer to pointer parameter, which could be deallocated / reallocated by getline.

I removed "BPAllocator.Reset", as this will be called anyway when the variable gets destroyed.

PStream is a FILE * stream, are you proposing using std::istream? That does not seem bring much benefit, except that I may omit that "fclose(PStream);" stmt.

207

lld::bAlloc won't be freed until after lld exits. I define "BPAllocator" so I have the freedom destroy the arena right after Propeller finishes marking CFGs.

217

I see. I'll change most of the maps into DenseMap, which shall be done in the next updated patch.

249

Yes, but to use std::default_delete, the class definition must be fully known, which for ELFView in this place, is not the case.
I use this ELFViewDeleter so I could move the definition of deleter to the .cpp, where ELFView is fully known. I intentionally avoid including ELFView.h in Propeller.h, which should not include any other Propeller headers.

ruiu added inline comments.Oct 1 2019, 12:11 AM
lld/ELF/Propeller.h
137

If getline doesn't work smoothly with a smart pointer, please consider not using that function. You may want to take a look at getLines() in lld/Common/Args.cpp.

shenhan updated this revision to Diff 222732.Oct 1 2019, 5:10 PM
shenhan marked 2 inline comments as done.
shenhan added inline comments.
lld/ELF/Propeller.h
137

Done by using std::ifstream. (getLines() in lld/Common/Args.cpp keeps all the lines in memory at the same time, which is not efficient for this scenario.)

ruiu added inline comments.Oct 2 2019, 12:37 AM
lld/ELF/Propeller.h
196

Please remove empty dtors from here and other classes.

lld/include/lld/Common/PropellerCommon.h
14

Importing from std is not allowed.

shenhan updated this revision to Diff 222948.Oct 2 2019, 6:18 PM
shenhan marked 2 inline comments as done.
ruiu added a comment.Oct 2 2019, 10:44 PM

Please run clang-format-diff to format this patch.

lld/ELF/Propeller.cpp
55

What does "if positive" mean?

130

The following format is more appropriate:

"<filename>:<linenumber>: Invalid ordinal field"

Please remove [Propeller] from the error messages. If users are using options for Propeller, they know what they are doing.

290

Generally, don't append this-> unless it is necessary.

lld/ELF/Propeller.h
15

I'd move this section below the main section. At this point, readers don't have any concrete idea what Propeller is, so explaining about misconceptions from the beginning doesn't make much sense.

26–29

Then, what is the name of the optimization you are implementing in this patch that is built on top of Propeller framework? Is it unnamed?

36

Here, you are using Propeller as a name of a concrete optimization technique rather than a name of a framework.

49

propeller

74

I think that this entire thing is called Propeller, and what Propeller class implements is some concrete optimization. So I think there must be a better name for that. What does the current "Propeller" class do? BB reordering?

128–131

Remove ELF from these class names. You can rename them later, but for now, you have only one implementation for ELF, so it is redundant.

137

You read each line individually, create a new std::string and make another copy in a StringSaver. That is probably not a good way to handle text files. I'd use mmap'ed file instead -- which is what MemoryBuffer provides.

138

Indent this part

171

Does 14 points to "main"? It looks like 14 points to "a" in the above example.

174

Where is symbol 17391?

193–194

You don't have to initialize members with (). You can remove them altogether.

196

This function is called only once, please inline.

197

Omit this-> if not necessary.

200

StringRef itself is a pointer-ish value, so pass a copy of StringRef instead of passing a reference to a StringRef.

205

Every nontrivial function needs a comment.

274

You are using this function only once. Please remove.

281–285

I'm puzzled by this -- do you really need this?

lld/include/lld/Common/PropellerCommon.h
2

This header is included only once by another header, so please merge them together.

20–23

I don't think this comment explains what this class represents. What is this class for?

32

Remove empty dtors.

shenhan updated this revision to Diff 223120.Oct 3 2019, 5:02 PM
shenhan marked 37 inline comments as done.
shenhan added inline comments.
lld/ELF/Propeller.cpp
55

Refined my comments.

130

Done by creating "Propfile::reportProfError" helper method.

lld/ELF/Propeller.h
26–29

Currently we have 3 kinds of optimizations - function splitting, basicblock reordering and function reordering. They are all based on BB sections and are implemented in "PropellerBBReordering.cpp/h & PropellerFuncOrdering.cpp/h" (in another CL). So we either call the optimization "BB Reordering based on Propeller" or we just call it "Propeller optimization", which covers all the optimization done within propeller framework, which for now is BB reordering only.

36

Actually I think this refers to the framework, because the description below centers around propeller's work, none of the actual optimization algorithms are covered.

74

Actually Propeller class does not do optimization at all. All optimizations are done in "PropellerBBReordering.cpp/h & PropellerFuncOrdering.cpp/h". What Propeller class does here is plumbing work - the interaction w/ lld, reading propeller profile (PropFile class), building and mapping CFG (ELFCfgBuilder class) and passing all these information to optimization passes ("PropellerBBReordering.cpp/h & PropellerFuncOrdering.cpp/h").

137

Yup, let me explain.

The std::string variable "line" is ephemeral, which only holds current line, it is destroyed after we move on to the next line. So we do not keep all the lines in memory at the same time.
We do extract part of the "line" content and save it to StrSaver, it's more efficient then use a MMAP, because it keeps all the lines in memory at the same time, all of which are not needed by Propeller (for example, the lines starting with "!", which are meant to be consumed by the compiler). Also for a line like "14 b 11.3" (ordinal-14, size-b, name: main.bb.3), we only save string "3" in the StringRef pool ("14" and "b" are converted to integers "11" are only used to find the "main" function, only "3" is saved.

171

s/14/11. Because I deleted some lines of the sample, and hadn't changed the line numbers in the text.

174

s/17391/19/

196

Added "inline" keyword. Or do you mean to inline and remove the function body?

274

This is also used in PropellerFuncOrdering.cpp file.

281–285

Yes, unique_ptr<Type> requires Type's dtor to be visible at this place. However, Propeller.h is the top level hdr file within propeller framework, thus it could not include any other propeller hdr file (only the other way around), so we don't have ELFView (now ObjectView after renaming)'s full type defintion here.

To overcome this, we provide a customer deleter functor - ELFViewDeleter, and define it in the ELFView's cpp file.

lld/include/lld/Common/PropellerCommon.h
2

Yup, let me explain. We have the creaet_llvm_prof tool which is part of google/autofdo repository (https://github.com/shenhanc78/autofdo/blob/plo-dev/llvm_propeller_profile_writer.h#L14). create_llvm_prof tool and propeller need to have exactly same symbolentry definition to cooperate. To make a copy of the class definition in google/autofdo repository is not a good idea. So we create this common inclusion hdr file.

20–23

Revisited the comments.

ruiu added inline comments.Oct 3 2019, 10:13 PM
lld/ELF/Propeller.cpp
133

Please follow the local convention -- if in doubt, please read other files to find out what we are doing there. Error messages shouldn't start with an uppercase letter, and shouldn't end with a full stop.

lld/ELF/Propeller.h
257

You should avoid using Prop or Propeller as a part of an identifier for Propeller, because essentially everything you are writing is for Propeller. So it's not very explanatory. For example, I'd name this reportParseError.

Please remove const from const StringRef.

ruiu added inline comments.Oct 3 2019, 10:26 PM
lld/ELF/Propeller.h
196

I meant the latter.

281–285

But I think you can fix that error by moving the definition of this class's ctor to a .cpp file (i.e. don't inline).

lld/include/lld/Common/PropellerCommon.h
2

This is an lld's private header directory. If you have a header that is shared by multiple LLVM subprojects, consider moving it to LLVM.

shenhan updated this revision to Diff 223256.Oct 4 2019, 11:24 AM
shenhan marked 8 inline comments as done.
shenhan added inline comments.
lld/ELF/Propeller.h
257

Yes. Also s/checkPropellerTarget/checkTarget/g & s/checkPropellerLegacy/checkLegacy/g.

281–285

Yes, alternatively, we can move Propeller's ctor *AND* dtor to Propeller.cpp, that also implies putting an empty "dtor" definition in the .cpp. Because compiler-synthesized dtors are inlined. What shall we do, keep the code as is, or move ctor and put an empty dtor in the .cpp? I am ok w/ either.

lld/include/lld/Common/PropellerCommon.h
2

I found PropellerCommon.h exported into the llvm installation directory here:

llvm-install/include/lld/Common

along with llvm-install/include/llvm and llvm-install/include/clang.

And I think it's probably inappropriate to place PropellerCommon.h anywhere under llvm-install/include/clang or llvm-install/include/llvm.

ruiu added inline comments.Oct 7 2019, 11:34 PM
lld/ELF/Propeller.cpp
55

If an object can be initialized by the default ctor, you can omit them from the initializer list, so please remove View() and CFGMap.

66–67

Is this how clang-format formatted? If not, could you please run clang-format-diff to format this patch?

82

Could you write a function comment for this function?

83

Could you write a comment as to what this magic string .llvm. is?

lld/ELF/Propeller.h
138

Could you indent these lines?

281–285

I think I prefer outlining the ctor and the dtor because it looks more natural.

lld/ELF/PropellerELFCfg.h
2

Remove ELF from this filename, as this file is already in ELF directory.

30

Just like other files, please leave a blank line after a file comment.

63

I think =0 is the default.

70

Use = INTRA_FUNC instead of an initializer list for consistency (it is important to keep the code consistent with other files.)

175–177

Please use = instead of {}

lld/include/lld/Common/PropellerCommon.h
2

So you are using this file only with in lld? If so, move this to lld/ELF, because we don't have a non-ELF implementation yet. Also, please consider rename SymbolEntry, even if this file is auto-generated by some other script. That name is extremely confusing in the linker's context. "Symbol" is one of the central data structures in the linker, and when you say "symbol", that means the symbol that we read from object files.

12

This line should be moved above using, just like other files.

MaskRay added inline comments.Oct 8 2019, 12:31 AM
lld/ELF/PropellerELFCfg.cpp
2

There are still open questions about the linker rewriting approach: https://lists.llvm.org/pipermail/llvm-dev/2019-October/135616.html

Let's see what conclusions people will reach.

20

Delete the comment.

I suspect a comment on its own line may interact badly with clang-format.

45

Use StringRef. Avoid const char *

std::error_code ec;
raw_fd_ostream os(..., ec, sys::fs::OF_None);
224

Delete unused code.

lld/ELF/PropellerELFCfg.h
96

delete the suffix l.

102

delete else

186

If the ordered property is not required, map -> unordered_map

229

not clang-format'ed, i.e. i don't see

} // namespace propeller
shenhan updated this revision to Diff 223918.Oct 8 2019, 12:06 PM
shenhan marked 22 inline comments as done.
shenhan added inline comments.
lld/ELF/Propeller.cpp
66–67

Ah, sorry, I didn't re-run clang-format after I made changes.

83

The special handling of ".llvm." in the symbols are no longer needed. Removed all such occurrences.

lld/ELF/PropellerELFCfg.cpp
2

Yup, we (Sri, Rahman and I) are working on replies for the points brought out by bolt engineers.

20

Deleted all comments regarding header inclusion.

lld/ELF/PropellerELFCfg.h
186

Yup, actually, the key of the map is the ordinal, which reflects the address order of each symbol. So here, the order property is required.

lld/include/lld/Common/PropellerCommon.h
2

No, I am sharing this file between lld and create_llvm_prof (which sits in another google repository - https://github.com/shenhanc78/autofdo/blob/plo-dev/llvm_propeller_profile_writer.h#L14 )

I'm thinking of renaming it to "BBSectionEntry", what do you think?

shenhan updated this revision to Diff 228939.Nov 12 2019, 12:19 PM

Hi, we've refactored propeller and cleaned up dependencies and interfaces. Here are the change summaries:

  1. all propeller files are moved into lld/ELF/Propeller, and lld/ELF/Propeller/* no longer depend on any lld/ELF header files (but they can depend on lld/include/lld/**)
  1. except 1 file - lld/ELF/LinkerPropeller.h&cpp, which defines interface between lld and propeller (and it can access lld/ELF/*.h files), all current and future interactions between these two must be defined here.
  1. lld/ELF/Driver.cpp now only has 1 call into propeller. (and other flag processing things.)

Please take another look, thanks.

ruiu added a comment.Nov 19 2019, 10:44 PM

Since you have moved all the files to a separate directory, it is probably better to create README.md to explain what Propeller is and how it works. You may want to move some file comments to that file.

shenhan updated this revision to Diff 230573.Nov 21 2019, 5:25 PM

Added lld/ELF/Propeller/README.md. Moved some of file comments into README.md.

shenhan updated this revision to Diff 238659.Jan 16 2020, 4:30 PM

Hello, over the past 2 months since our first draft of Propeller, we've made huge changes/improvements and this describes what happens -http://lists.llvm.org/pipermail/llvm-dev/2020-January/138426.html

I've also updated the lld patch, please take another look, thanks lot!

-Han