[PATCH] D68062: Propeller lld framework for basicblock sections

Wed Oct 2 22:45:06 PDT 2019

ruiu added a comment.

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

================
Comment at: lld/ELF/Propeller.cpp:54
+// Read the "@" directive in the propeller file, compare it against "-o"
+// filename, return true if positive.
+bool Propfile::matchesOutputFileName(const StringRef &outputFileName) {
----------------
What does "if positive" mean?

================
Comment at: lld/ELF/Propeller.cpp:129
+        SOrdinal == 0) {
+      error("[Propeller]: Invalid ordinal field, at propfile line: " +
+            std::to_string(LineNo) + ".");
----------------
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.

================
Comment at: lld/ELF/Propeller.cpp:289
+      std::lock_guard<std::mutex> lock(this->Lock);
+      this->Views.emplace_back(View);
+      for (auto &P : View->CFGs) {
----------------
Generally, don't append `this->` unless it is necessary.

================
Comment at: lld/ELF/Propeller.h:136
+                                    uint64_t Size) {
+    auto *Sym = new SymbolEntry(Ordinal, Name, std::move(Aliases),
+                                SymbolEntry::INVALID_ADDRESS, Size,
----------------
shenhan wrote:
> ruiu wrote:
> > shenhan wrote:
> > > 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.
> > 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.
> 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.)
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.

================
Comment at: lld/ELF/Propeller.h:14
+//
+// Some misconections:
+//
----------------
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.

================
Comment at: lld/ELF/Propeller.h:25-28
+// The above statement is not correct. Propeller is a *general*
+// framework, it will be able to do things like reodering individual
+// insns, remove/insert spill insns, etc. And we have plans for future
+// optimizations based on propeller framework (not on bb sections).
----------------
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?

================
Comment at: lld/ELF/Propeller.h:35
+//
+// B. How (current) propeller works.
+//
----------------
Here, you are using Propeller as a name of a concrete optimization technique rather than a name of a framework.

================
Comment at: lld/ELF/Propeller.h:48
+//
+// LLD handles execution to propelle (Propeller.h is the interface
+// that works with lld), which does a few things for each ELF object
----------------
propeller

================
Comment at: lld/ELF/Propeller.h:73
+//
+//   Propeller - the main propeller framework class
+//
----------------
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?

================
Comment at: lld/ELF/Propeller.h:127-130
+class ELFCFG;
+class ELFCFGEdge;
+class ELFCFGNode;
+class ELFView;
----------------
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.

================
Comment at: lld/ELF/Propeller.h:137
+//
+// Symbols
+// 1 0 N.init/_init
----------------
Indent this part

================
Comment at: lld/ELF/Propeller.h:170
+//              identification string (could be an index number). For the above
+//              example, name "14.2" means "main.bb.2", because "14" points to
+//              symbol main. Also note, symbols could have aliases, in such
----------------
Does 14 points to "main"? It looks like 14 points to "a" in the above example.

================
Comment at: lld/ELF/Propeller.h:173
+//              case, aliases are concatenated with the original name with a
+//              '/'. For example, symbol 17391 contains 2 aliases.
+// Note, the symbols listed are in strict non-decreasing address order.
----------------
Where is symbol 17391?

================
Comment at: lld/ELF/Propeller.h:192-193
+  Propfile(Propeller &p, const std::string &pName)
+      : BPAllocator(), PropfileStrSaver(BPAllocator), PropfName(pName),
+        PropfStream(), Prop(p), SymbolOrdinalMap() {}
+
----------------
You don't have to initialize members with `()`. You can remove them altogether.

================
Comment at: lld/ELF/Propeller.h:195
+
+  bool openPropf() {
+    this->PropfStream.open(this->PropfName);
----------------
This function is called only once, please inline.

================
Comment at: lld/ELF/Propeller.h:196
+  bool openPropf() {
+    this->PropfStream.open(this->PropfName);
+    return this->PropfStream.good();
----------------
Omit `this->` if not necessary.

================
Comment at: lld/ELF/Propeller.h:199
+  }
+  bool matchesOutputFileName(const StringRef &outputFile);
+  bool readSymbols();
----------------
`StringRef` itself is a pointer-ish value, so pass a copy of StringRef instead of passing a reference to a StringRef.

================
Comment at: lld/ELF/Propeller.h:204
+
+  SymbolEntry *createFunctionSymbol(uint64_t ordinal, const StringRef &name,
+                                    SymbolEntry::AliasesTy &&aliases,
----------------
Every nontrivial function needs a comment.

================
Comment at: lld/ELF/Propeller.h:273
+  template <class Visitor>
+  void forEachCfgRef(Visitor v) {
+    for (auto &p : CFGMap)
----------------
You are using this function only once. Please remove.

================
Comment at: lld/ELF/Propeller.h:280-284
+  // ELFViewDeleter, which has its implementation in .cpp, saves us from having
+  // to have full ELFView definition visibile here.
+  struct ELFViewDeleter {
+    void operator()(ELFView *v);
+  };
----------------
I'm puzzled by this -- do you really need this?

================
Comment at: lld/include/lld/Common/PropellerCommon.h:1
+#ifndef LLD_ELF_PROPELLER_COMMON_H
+#define LLD_ELF_PROPELLER_COMMON_H
----------------
This header is included only once by another header, so please merge them together.

================
Comment at: lld/include/lld/Common/PropellerCommon.h:19-22
+// This data structure is shared between lld propeller component and
+// create_llvm_prof.
+// The basic block symbols are encoded in this way:
+//    index.'bb'.function_name
----------------
I don't think this comment explains what this class represents. What is this class for?

================
Comment at: lld/include/lld/Common/PropellerCommon.h:31
+        BBTag(BB), ContainingFunc(FuncPtr) {}
+  ~SymbolEntry() {}
+
----------------
Remove empty dtors.

Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D68062