[PATCH] D68062: Propeller lld framework for basicblock sections
Han Shen via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 30 17:28:50 PDT 2019
shenhan added inline comments.
================
Comment at: lld/ELF/Propeller.cpp:27
+#include "Config.h"
+#include "PropellerBBReordering.h"
+#include "PropellerELFCfg.h"
----------------
ruiu wrote:
> shenhan wrote:
> > MaskRay wrote:
> > > PropellerBBReordering.h is added in D68073, not in this patch. Please make each patch compilable.
> > 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?
> 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.
Thanks. Yes, that makes sense.
================
Comment at: lld/ELF/Propeller.cpp:27
+#include "Config.h"
+#include "PropellerBBReordering.h"
+#include "PropellerELFCfg.h"
----------------
shenhan wrote:
> ruiu wrote:
> > shenhan wrote:
> > > MaskRay wrote:
> > > > PropellerBBReordering.h is added in D68073, not in this patch. Please make each patch compilable.
> > > 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?
> > 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.
> Thanks. Yes, that makes sense.
We will merge 3 patches into 1 and submit in one shot.
================
Comment at: lld/ELF/Propeller.cpp:51
+
+using std::string;
+using std::tuple;
----------------
MaskRay wrote:
> Delete using std::$container.
Deleted all "std:*".
================
Comment at: lld/ELF/Propeller.h:20
+
+using std::list;
+using std::map;
----------------
MaskRay wrote:
> MaskRay wrote:
> > Delete such `using std::$container;`
> There are still lots of `using std::string` or `using std::foobar` here and there.
Removed all "using std::*", and lots of "using llvm::*".
================
Comment at: lld/ELF/Propeller.h:136
+ uint64_t Size) {
+ auto *Sym = new SymbolEntry(Ordinal, Name, std::move(Aliases),
+ SymbolEntry::INVALID_ADDRESS, Size,
----------------
MaskRay wrote:
> Prefer unique_ptr to raw pointers.
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.
================
Comment at: lld/ELF/Propeller.h:9-13
+// Propeller.h defines Propeller framework classes:
+//
+// Propfile - parses and wraps propeller profile
+//
+// Propeller - the main propeller framework class
----------------
ruiu wrote:
> 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.
Thanks. I pre-pended paragraphs w/ more details to this part.
================
Comment at: lld/ELF/Propeller.h:9
+//
+// Propeller.h defines Propeller framework classes:
+//
----------------
ruiu wrote:
> 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...
Thanks. I added a section "About propeller framework" at the top of Propeller.h.
================
Comment at: lld/ELF/Propeller.h:17
+// (Propeller::checkPropellerTarget), then it enters Propeller::processFiles(),
+// which builds Cfg for each valid ELF object file (Propeller::processFile ->
+// ELFCfgBuilder::buildCfgs).
----------------
ruiu wrote:
> 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.
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"...)
================
Comment at: lld/ELF/Propeller.h:20
+//
+// After Cfgs are build, Propeller starts parsing Propeller profile (the
+// Propfile class). In this step, bb symbols are created, branch/call counters
----------------
ruiu wrote:
> CFGs
>
> (but is plural correct?)
Replaced CFGs w/ "control flow graphs".
================
Comment at: lld/ELF/Propeller.h:72
+
+class ELFCfg;
+class ELFCfgEdge;
----------------
ruiu wrote:
> Do you need these `ELF` prefixes even if they are in `lld::elf` namespace?
>
> Cfg -> CFG
Actually these are in lld::propeller namespace.
================
Comment at: lld/ELF/Propeller.h:158-163
+ if (LineBuf) {
+ free(LineBuf);
+ }
+ BPAllocator.Reset();
+ if (PStream) fclose(PStream);
+ }
----------------
ruiu wrote:
> 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.
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.
================
Comment at: lld/ELF/Propeller.h:206
+
+ llvm::BumpPtrAllocator BPAllocator;
+ llvm::UniqueStringSaver PropfileStrSaver;
----------------
ruiu wrote:
> You can use `lld::bAlloc` instead of defining your own.
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.
================
Comment at: lld/ELF/Propeller.h:216
+ // etc...
+ map<StringRef, map<StringRef, SymbolEntry *>> SymbolNameMap;
+ list<SymbolEntry*> FunctionsWithAliases;
----------------
ruiu wrote:
> std::map is generally slower than llvm::DenseMap. Do you need to use std::map?
I see. I'll change most of the maps into DenseMap, which shall be done in the next updated patch.
================
Comment at: lld/ELF/Propeller.h:248
+
+ // ELFViewDeleter, which has its implementation in .cpp, saves us from having
+ // to have full ELFView definition visibile here.
----------------
MaskRay wrote:
> std::unique_ptr defaults to std::default_delete.
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.
================
Comment at: lld/ELF/PropellerELFCfg.h:156
+ void emplaceEdge(ELFCfgEdge *Edge) {
+ if (Edge->Type < ELFCfgEdge::INTER_FUNC_CALL) {
+ IntraEdges.emplace_back(Edge);
----------------
MaskRay wrote:
> MaskRay wrote:
> > Delete `{}`
> >
> > This is applicable to many places of the patch series.
> Still lots of `{}` surrounding simple statements.
BTW, I intentionally keep "{}" in these scenarios, if that makes sense to you.
for (...) {
if (...) {
for (...)
xxx
} else {
yyy
}
}
Repository:
rLLD LLVM Linker
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68062/new/
https://reviews.llvm.org/D68062
More information about the llvm-commits
mailing list