[PATCH] D68073: Propeller code layout optimizations

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Let me start with high-level comments:

- Your code looks unnecessarily different from existing code from the coding style viewpoint. Please read other files and follow the same coding style.
- Please run clang-format-diff to format this patch.
- It lacks comments. In particular, it needs a file comment explaining the algorithm. Once you successfully explain the algorithm, each function/data should become more self-explanatory.



================
Comment at: lld/ELF/PropellerBBReordering.cpp:1
+#include "PropellerBBReordering.h"
+
----------------
Please add a file comment describing the algorithm. An example of it is https://github.com/llvm/llvm-project/blob/master/lld/ELF/ICF.cpp. Just like that file, please explain the algorithm.


================
Comment at: lld/ELF/PropellerBBReordering.cpp:25
+
+ostream &operator<<(ostream &Out, const NodeChain &Chain) {
+  Out << "Chain for Cfg Node: {" << *(Chain.DelegateNode) << "}: total binary size: " << Chain.Size
----------------
Define `lld::toString(NodeChain &)` instead of overloading `operator<<`.


================
Comment at: lld/ELF/PropellerBBReordering.h:1
+#ifndef LLD_ELF_PROPELLER_BB_REORDERING_H
+#define LLD_ELF_PROPELLER_BB_REORDERING_H
----------------
Add the standard file header comment.


================
Comment at: lld/ELF/PropellerBBReordering.h:12-18
+using std::list;
+using std::map;
+using std::set;
+using std::unique_ptr;
+using std::unordered_map;
+using std::unordered_set;
+using std::vector;
----------------
It is not permitted to import identifiers from `std` by the LLVM coding style.


================
Comment at: lld/ELF/PropellerBBReordering.h:20
+
+using llvm::StringMap;
+
----------------
Instead of doing this, add `#include "lld/Common/LLVM.h"` just like other files do.


================
Comment at: lld/ELF/PropellerBBReordering.h:35
+
+/* Represents a chain of ELFCfgNodes (basic blocks). */
+class NodeChain {
----------------
We use `//` comments.


================
Comment at: lld/ELF/PropellerBBReordering.h:39
+  const ELFCfgNode *DelegateNode = nullptr;
+  list<const ELFCfgNode *> Nodes;
+
----------------
Is there any reason you can't use std::vector? std::list is generally slower than std::vector for most operations, though inserting/removing a new element in a middle of a long list is faster.


================
Comment at: lld/ELF/PropellerBBReordering.h:42
+  // Total binary size of the chain
+  uint32_t Size{0};
+
----------------
Please read the other files in the same directory and follow the same style, so that this code doesn't unecessarily look different. This should be `uint32_t Size = 0`;


================
Comment at: lld/ELF/PropellerBBReordering.h:109
+public:
+  virtual ~NodeChainBuilder() = default;
+  NodeChainBuilder(NodeChainBuilder &) = delete;
----------------
Why do you need a virtual dtor?


================
Comment at: lld/ELF/PropellerFuncOrdering.h:24
+    Cluster(ELFCfg *Cfg);
+    ~Cluster();
+    list<ELFCfg *> Cfgs;
----------------
Do you need to define a dtor?


================
Comment at: lld/ELF/PropellerFuncOrdering.h:25-27
+    list<ELFCfg *> Cfgs;
+    uint64_t       Size;
+    uint64_t       Weight;
----------------
Please run clang-format-diff on this patch.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D68073