[PATCH] D68062: Propeller lld framework for basicblock sections

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

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.



================
Comment at: lld/ELF/Propeller.h:9
+//
+// Propeller.h defines Propeller framework classes:
+//
----------------
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...


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


================
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
----------------
CFGs

(but is plural correct?)


================
Comment at: lld/ELF/Propeller.h:21
+// After Cfgs are build, Propeller starts parsing Propeller profile (the
+// Propfile class). In this step, bb symbols are created, branch/call counters
+// are read, and *very importantly*, the counters are applied to the Cfg. For
----------------
bb -> BB


================
Comment at: lld/ELF/Propeller.h:57
+
+using std::list;
+using std::map;
----------------
Remove these `using` directives. It is not allowed to import names from std in the LLVM coding style.


================
Comment at: lld/ELF/Propeller.h:72
+
+class ELFCfg;
+class ELFCfgEdge;
----------------
Do you need these `ELF` prefixes even if they are in `lld::elf` namespace?

Cfg -> CFG


================
Comment at: lld/ELF/Propeller.h:80
+//
+// a sample propeller profile is like below:
+//
----------------
a -> A


================
Comment at: lld/ELF/Propeller.h:83
+// Symbols
+// 1 0 N.init/_init
+// 2 0 N.plt
----------------
Please reduce the number of lines of this example so that the example contains just enough information to explain your idea.


================
Comment at: lld/ELF/Propeller.h:158-163
+    if (LineBuf) {
+      free(LineBuf);
+    }
+    BPAllocator.Reset();
+    if (PStream) fclose(PStream);
+  }
----------------
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.


================
Comment at: lld/ELF/Propeller.h:206
+
+  llvm::BumpPtrAllocator BPAllocator;
+  llvm::UniqueStringSaver PropfileStrSaver;
----------------
You can use `lld::bAlloc` instead of defining your own.


================
Comment at: lld/ELF/Propeller.h:216
+  //   etc...
+  map<StringRef, map<StringRef, SymbolEntry *>> SymbolNameMap;
+  list<SymbolEntry*> FunctionsWithAliases;
----------------
std::map is generally slower than llvm::DenseMap. Do you need to use std::map?


================
Comment at: lld/ELF/Propeller.h:218-221
+  uint64_t LineNo;
+  char     LineTag;
+  size_t   LineSize;
+  char    *LineBuf;
----------------
Format


================
Comment at: lld/ELF/Propeller.h:264-265
+  unique_ptr<Propfile> Propf;
+  // Lock to access / modify global data structure.
+  mutex Lock;
+};
----------------
Mutex is used for locking, so this comment isn't useful. Please explain what resource you actually want to guard using this lock.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68062/new/

https://reviews.llvm.org/D68062