[PATCH] D129518: [BOLT] Add function layout class

[Maksim Panchenko via Phabricator via llvm-commits]

maksfb added inline comments.


================
Comment at: bolt/include/bolt/Core/BinaryFunction.h:1378
   /// Return true if the function body is non-contiguous.
   bool isSplit() const {
+    return isSimple() && !getLayout().block_empty() &&
----------------
What do you think of making `isSplit()` a property of `FunctionLayout`? We can keep this method as it also uses `isSimple()` for the sake of NFC, but perhaps this check is unnecessary.


================
Comment at: bolt/include/bolt/Core/FunctionLayout.h:79
+///
+/// Interally, the function layout stores blocks across fragments contiguous.
+/// This is neccessary to retain compatilibity with existing code and tests that
----------------



================
Comment at: bolt/include/bolt/Core/FunctionLayout.h:176
+  /// Returns the basic block after the given basic block in the layout or
+  /// nullptr the last basic block is given.
+  BinaryBasicBlock *getBasicBlockAfter(const BinaryBasicBlock *BB,
----------------



================
Comment at: bolt/lib/Core/BinaryEmitter.cpp:401
   bool FirstInstr = true;
-  for (BinaryBasicBlock *BB : BF.layout()) {
+  for (BinaryBasicBlock *BB : BF.getLayout().blocks()) {
     if (EmitColdPart != BB->isCold())
----------------
We can take advantage of the new layout class and iterate over fragments here.


================
Comment at: bolt/lib/Core/BinaryFunction.cpp:3220
     // Basic block that follows the current one in the final layout.
-    const BinaryBasicBlock *NextBB = nullptr;
-    if (I + 1 != E && BB->isCold() == BasicBlocksLayout[I + 1]->isCold())
-      NextBB = BasicBlocksLayout[I + 1];
+    const BinaryBasicBlock *NextBB = Layout.getBasicBlockAfter(BB, false);
 
----------------



================
Comment at: bolt/lib/Core/BinaryFunction.cpp:3555
+  else
+    copy(Layout.blocks(), std::back_inserter(Order));
 
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129518