[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:
> > 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)?
> > 
> > 
> I am not sure I understand the question, could you clarify?
> 
> The weights affect the resulting layout, which in turn may impact perf. However, changing the weights from 0.1 to say 0.2 likely won't make a difference, except for some corner cases.
> IIRC, in my tests 3 years ago it was important to keep `ForwardWeight<FallthroughWeight=1.0`. Intuitively this makes sense, though the exact values may need to be re-tuned. (This is a separate and time-consuming project)
You answer touched upon what I was asking.  To clarify the question: are there performance data to show why 0.1 (instead of other values) is used.  ExtTSP score can be considered as a  locality score, and the forwardWeight is an attempt to model the cost of taken branch instruction.   I wonder if there is a better way to model this in the objective function. No need to address it in this patch though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113424