[PATCH] D113424: ext-tsp basic block layout

[David Li via Phabricator via llvm-commits]

davidxl 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;
+    }
----------------
spupyrev wrote:
> 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
I guessed that with ForwardWeight == 0.1,  a bias can be introduced against laying out larger successor as the fall through block, but it is not the case. Using different forward weight seems to produce the same decision for a simple diamond cfg case  -- in order to for one successor to be a fallthrough, the branch probability needs to > 50%.

I wonder what role this weight plays in practice (performance)?




================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:926
+  for (size_t Idx = 0; Idx < Order.size(); Idx++) {
+    BlockIndex[Order[Idx]] = Idx;
+  }
----------------
spupyrev wrote:
> davidxl wrote:
> > From the initialization, Order[Idx] == Idx, so is this map needed?
> This function is also called when Order != Identity
Probably provide another wrapper to compute the mapping and pass it in. The map can use the vector type, so for the case when Order == Identity, Order can be used for both.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113424