[PATCH] D68062: Propeller lld framework for basicblock sections

Mon Oct 7 23:34:43 PDT 2019

ruiu added inline comments.

================
Comment at: lld/ELF/Propeller.cpp:54
+Propeller::Propeller(lld::elf::SymbolTable *ST)
+    : Symtab(ST), Views(), CFGMap(), Propf(nullptr) {}
+
----------------
If an object can be initialized by the default ctor, you can omit them from the initializer list, so please remove `View()` and `CFGMap`.

================
Comment at: lld/ELF/Propeller.cpp:65-66
+    ++LineNo;
+    if (line.empty()) continue;
+    if (line[0] != '@') break;
+    ++outputFileTagSeen;
----------------
Is this how clang-format formatted? If not, could you please run clang-format-diff to format this patch?

================
Comment at: lld/ELF/Propeller.cpp:81
+
+SymbolEntry *Propfile::findSymbol(StringRef symName) {
+  std::pair<StringRef, StringRef> symNameSplit = symName.split(".llvm.");
----------------
Could you write a function comment for this function?

================
Comment at: lld/ELF/Propeller.cpp:82
+SymbolEntry *Propfile::findSymbol(StringRef symName) {
+  std::pair<StringRef, StringRef> symNameSplit = symName.split(".llvm.");
+  StringRef funcName;
----------------
Could you write a comment as to what this magic string `.llvm.` is?

================
Comment at: lld/ELF/Propeller.h:137
+//
+// Symbols
+// 1 0 N.init/_init
----------------
ruiu wrote:
> Indent this part
Could you indent these lines?

================
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);
+  };
----------------
shenhan wrote:
> ruiu wrote:
> > shenhan wrote:
> > > 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.
> > 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).
> 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.
I think I prefer outlining the ctor and the dtor because it looks more natural.

================
Comment at: lld/ELF/PropellerELFCfg.h:1
+//===-------------------- PropellerELFCfg.h -------------------------------===//
+//
----------------
Remove ELF from this filename, as this file is already in ELF directory.

================
Comment at: lld/ELF/PropellerELFCfg.h:29
+//===----------------------------------------------------------------------===//
+#ifndef LLD_ELF_PROPELLER_ELF_CFG_H
+#define LLD_ELF_PROPELLER_ELF_CFG_H
----------------
Just like other files, please leave a blank line after a file comment.

================
Comment at: lld/ELF/PropellerELFCfg.h:62
+  enum EdgeType : char {
+    INTRA_FUNC = 0,
+    INTRA_RSC,
----------------
I think `=0` is the default.

================
Comment at: lld/ELF/PropellerELFCfg.h:69
+    INTER_FUNC_RETURN,
+  } Type{INTRA_FUNC};
+
----------------
Use `= INTRA_FUNC` instead of an initializer list for consistency (it is important to keep the code consistent with other files.)

================
Comment at: lld/ELF/PropellerELFCfg.h:174-176
+  uint32_t BB{0};
+  uint32_t BBWoutAddr{0};
+  uint32_t InvalidCFGs{0};
----------------
Please use `=` instead of `{}`

================
Comment at: lld/include/lld/Common/PropellerCommon.h:11
+
+#include <string>
+
----------------
This line should be moved above `using`, just like other files.

================
Comment at: lld/include/lld/Common/PropellerCommon.h:1
+#ifndef LLD_ELF_PROPELLER_COMMON_H
+#define LLD_ELF_PROPELLER_COMMON_H
----------------
shenhan wrote:
> ruiu wrote:
> > shenhan wrote:
> > > 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.
> > > 
> > > 
> > 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.
> 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.
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.

Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D68062