[PATCH] D68073: Propeller code layout optimizations

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/ELF/PropellerBBReordering.cpp:9
+//
+// This file is part of the Propeller infrastcture for doing code layout
+// optimization and includes the implementation of intra-function basic block
----------------
infrastructure


================
Comment at: lld/ELF/PropellerBBReordering.cpp:27
+//   *  0.1 * (1 - distance(Src[e], Sink[e]) / 1024) if Src[e] < Sink[e] and
+//   0 < distance(Src[e], Sink[e]) < 1024 (i.e. short forward jump)
+//   *  0.1 * (1 - distance(Src[e], Sink[e]) / 640) if Src[e] > Sink[e] and
----------------
nit: add two spaces so that the second line of a list item is properly indented.


================
Comment at: lld/ELF/PropellerBBReordering.cpp:80-81
+
+#include <stdio.h>
+
+#include <numeric>
----------------
What function are you using from stdio.h?


================
Comment at: lld/ELF/PropellerBBReordering.cpp:120
+    std::vector<const NodeChain *> &chainOrder) {
+  for (auto CI = Chains.begin(), CE = Chains.end(); CI != CE; ++CI) {
+    chainOrder.push_back(CI->second.get());
----------------
You can use the foreach-style for loop here: `for (std::pair<int64_t, std::unique_ptr<NodeChain>> Elem : Chains)`


================
Comment at: lld/ELF/PropellerBBReordering.cpp:192
+
+  // It's only possible to form a fall-through between src and sink if src is
+  // they are respectively located at the end and beginning of their chains.
----------------
src is they are ...?


================
Comment at: lld/ELF/PropellerBBReordering.cpp:619-622
+        config->symbolAlignmentFile.insert(std::make_pair(n->ShName, 16));
+      } else
+        config->symbolAlignmentFile.insert(std::make_pair(n->ShName, 1));
+    }
----------------
It took for a while to find this line. This is the output of this ordering algorithm, right? Is there any other output from this algorithm?


================
Comment at: lld/ELF/PropellerBBReordering.h:44-48
+  // Total binary size of the chain
+  uint32_t Size = 0;
+
+  // Total execution frequency of the chain
+  uint64_t Freq = 0;
----------------
Because they are always initialized by the constructor, they shouldn't have default values.


================
Comment at: lld/ELF/PropellerBBReordering.h:54-59
+  NodeChain(const ELFCFGNode *Node) {
+    DelegateNode = Node;
+    Nodes.push_back(Node);
+    Size = Node->ShSize;
+    Freq = Node->Freq;
+  }
----------------
In C++, it is generally preferred to initialize members from the beginning using an initializer list instead of updating members after they are constructed. So,

  NodeChain(const ELFCFGNode *Node) : DelegateNode(Node), Size(Node->ShSize), Freq(Node->Freq) {
    Nodes.push_back(Node);
  }


================
Comment at: lld/ELF/PropellerBBReordering.h:65-67
+  const ELFCFGNode *getFirstNode() const { return Nodes.front(); }
+
+  const ELFCFGNode *getLastNode() const { return Nodes.back(); }
----------------
Do you really have to define these functions, given that you can just access Nodes to do the same thing?


================
Comment at: lld/ELF/PropellerFuncOrdering.cpp:2-9
+//-------------------------------------------===//
+////
+//// Part of the LLVM Project, under the Apache License v2.0 with LLVM
+// Exceptions.
+//// See https://llvm.org/LICENSE.txt for license information.
+//// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+////
----------------
Formatting doesn't seem correct.


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

https://reviews.llvm.org/D68073