[PATCH] D113424: ext-tsp basic block layout

[Sergey Pupyrev via Phabricator via llvm-commits]

spupyrev added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:97
+      double Prob = 1.0 - static_cast<double>(Dist) / ForwardDistance;
+      return ForwardWeight * Prob * Count;
+    }
----------------
davidxl wrote:
> The tsp score does not seem to be contiguous here. When Dist == 0, it should have the same score as the fall through case, but this produces 0.1* Count.
Agree, that's something me and my colleagues have been thinking about lately.

The current function is tuned for the maximum performance of the resulting binary. It is not guaranteed/enforced that the function is contiguous. That said, i'd be super happy to see and review improvements of the objective in the future. There is likely a room for optimizations here


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:926
+  for (size_t Idx = 0; Idx < Order.size(); Idx++) {
+    BlockIndex[Order[Idx]] = Idx;
+  }
----------------
davidxl wrote:
> From the initialization, Order[Idx] == Idx, so is this map needed?
This function is also called when Order != Identity


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:939
+  }
+  return Score;
+}
----------------
davidxl wrote:
> This function computes the 'fallthrough' maximizing TSP score, not the extTSP in the original paper. Where is the h function and the K coeffcients?
oops. Refactored function to compute correct result


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113424