[PATCH] D68062: Propeller lld framework for basicblock sections

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Propeller.cpp:531
+    algo.init(*this);
+    auto startFuncOrderTime = system_clock::now();
+    auto cfgsReordered = algo.doOrder(cfgOrder);
----------------
Delete adhoc timers. There is currently no such timer mechanism in lld, but if we do, we should use proper measuring mechanisms like llvm::TimeRegion, and have a user interface like gcc/clang -ftime-record, not ad-hoc stdout dumps.


================
Comment at: lld/ELF/Propeller.h:1
+#ifndef LLD_ELF_PROPELLER_H
+#define LLD_ELF_PROPELLER_H
----------------
shenhan wrote:
> MaskRay wrote:
> > MaskRay wrote:
> > > Add license notice.
> > > Use variableName instead of VariableName to conform to the local naming convention.
> > > Use variableName instead of VariableName to conform to the local naming convention.
> > 
> > This is not done.
> A little bit confused. https://llvm.org/docs/CodingStandards.html states that noun name should be camel cased, with first letter in upper case. However I also see LLD files have used "smallUpperCaseNames" as locals...
> 
> I'll conform to lld.
LLD's variable naming convention was changed in D64121.


================
Comment at: lld/ELF/Propeller.h:20
+
+using std::list;
+using std::map;
----------------
MaskRay wrote:
> Delete such `using std::$container;`
There are still lots of `using std::string` or `using std::foobar` here and there.


================
Comment at: lld/ELF/Propeller.h:248
+
+  // ELFViewDeleter, which has its implementation in .cpp, saves us from having
+  // to have full ELFView definition visibile here.
----------------
std::unique_ptr defaults to std::default_delete.


================
Comment at: lld/ELF/PropellerELFCfg.h:63
+
+class ELFCfgEdge {
+public:
----------------
The interface here, and the one in the hfsort BBreordering patch are overly complex. Some abstractions are not really needed. Delete `friend` declarations, make some `class`es struct, and migrate from `std::list` to more efficient containers. There should be a rethink how the graph theory abstractions are connected.


================
Comment at: lld/ELF/PropellerELFCfg.h:238
+
+ostream & operator << (ostream &out, const ELFCfgNode &node);
+ostream & operator << (ostream &out, const ELFCfgEdge &edge);
----------------
Not clang-format'ed.


================
Comment at: lld/ELF/PropellerELFCfg.h:156
+  void emplaceEdge(ELFCfgEdge *Edge) {
+    if (Edge->Type < ELFCfgEdge::INTER_FUNC_CALL) {
+      IntraEdges.emplace_back(Edge);
----------------
MaskRay wrote:
> Delete `{}`
> 
> This is applicable to many places of the patch series.
Still lots of `{}` surrounding simple statements.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D68062