[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:65-66
+    ++LineNo;
+    if (line.empty()) continue;
+    if (line[0] != '@') break;
+    ++outputFileTagSeen;
----------------
ruiu wrote:
> Is this how clang-format formatted? If not, could you please run clang-format-diff to format this patch?
Ah, sorry, I didn't re-run clang-format after I made changes.


================
Comment at: lld/ELF/Propeller.cpp:82
+SymbolEntry *Propfile::findSymbol(StringRef symName) {
+  std::pair<StringRef, StringRef> symNameSplit = symName.split(".llvm.");
+  StringRef funcName;
----------------
ruiu wrote:
> Could you write a comment as to what this magic string `.llvm.` is?
The special handling of ".llvm." in the symbols are no longer needed. Removed all such occurrences.



================
Comment at: lld/ELF/PropellerELFCfg.cpp:1
+//===-------------------- PropellerELFCfg.cpp -----------------------------===//
+//
----------------
MaskRay wrote:
> There are still open questions about the linker rewriting approach: https://lists.llvm.org/pipermail/llvm-dev/2019-October/135616.html
> 
> Let's see what conclusions people will reach.
Yup, we (Sri, Rahman and I) are working on replies for the points brought out by bolt engineers.


================
Comment at: lld/ELF/PropellerELFCfg.cpp:19
+#include "llvm/Object/ObjectFile.h"
+// Needed by ELFSectionRef & ELFSymbolRef.
+#include "llvm/Object/ELFObjectFile.h"
----------------
MaskRay wrote:
> Delete the comment.
> 
> I suspect a comment on its own line may interact badly with clang-format.
Deleted all comments regarding header inclusion.


================
Comment at: lld/ELF/PropellerELFCfg.h:185
+  // See implementaion comments in .cpp.
+  void buildCFG(ControlFlowGraph &cfg, const SymbolRef &cfgSym,
+                std::map<uint64_t, std::unique_ptr<CFGNode>> &nodeMap);
----------------
MaskRay wrote:
> If the ordered property is not required, map -> unordered_map
Yup, actually, the key of the map is the ordinal, which reflects the address order of each symbol. So here, the order property is required.


================
Comment at: lld/include/lld/Common/PropellerCommon.h:1
+#ifndef LLD_ELF_PROPELLER_COMMON_H
+#define LLD_ELF_PROPELLER_COMMON_H
----------------
ruiu wrote:
> 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.
No, I am sharing this file between lld and create_llvm_prof (which sits in another google repository - https://github.com/shenhanc78/autofdo/blob/plo-dev/llvm_propeller_profile_writer.h#L14 )

I'm thinking of renaming it to "BBSectionEntry", what do you think?


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D68062