[PATCH] D68062: Propeller lld framework for basicblock sections

[Han Shen via Phabricator via llvm-commits]

shenhan added inline comments.


================
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) {
----------------
ruiu wrote:
> What does "if positive" mean?
Refined my comments.


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


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



================
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).
----------------
ruiu wrote:
> 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?
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.


================
Comment at: lld/ELF/Propeller.h:35
+//
+// B. How (current) propeller works.
+//
----------------
ruiu wrote:
> Here, you are using Propeller as a name of a concrete optimization technique rather than a name of a framework.
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.


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


================
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
----------------
ruiu wrote:
> Does 14 points to "main"? It looks like 14 points to "a" in the above example.
s/14/11. Because I deleted some lines of the sample, and hadn't changed the line numbers in the text.


================
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.
----------------
ruiu wrote:
> Where is symbol 17391?
s/17391/19/


================
Comment at: lld/ELF/Propeller.h:195
+
+  bool openPropf() {
+    this->PropfStream.open(this->PropfName);
----------------
ruiu wrote:
> This function is called only once, please inline.
Added "inline" keyword. Or do you mean to inline and remove the function body?


================
Comment at: lld/ELF/Propeller.h:273
+  template <class Visitor>
+  void forEachCfgRef(Visitor v) {
+    for (auto &p : CFGMap)
----------------
ruiu wrote:
> You are using this function only once. Please remove.
This is also used in PropellerFuncOrdering.cpp file.


================
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);
+  };
----------------
ruiu wrote:
> I'm puzzled by this -- do you really need this?
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.


================
Comment at: lld/include/lld/Common/PropellerCommon.h:1
+#ifndef LLD_ELF_PROPELLER_COMMON_H
+#define LLD_ELF_PROPELLER_COMMON_H
----------------
ruiu wrote:
> This header is included only once by another header, so please merge them together.
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.




================
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
----------------
ruiu wrote:
> I don't think this comment explains what this class represents. What is this class for?
Revisited the comments.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D68062