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

[Fabian Parzefall via Phabricator via llvm-commits]

FPar 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() &&
----------------
maksfb wrote:
> 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.
I agree.


================
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())
----------------
maksfb wrote:
> We can take advantage of the new layout class and iterate over fragments here.
I already have considered something like this:


```
const FunctionFragment F = BF.getLayout().getFragment(
      EmitColdPart ? FragmentNum::cold() : FragmentNum::hot());
  for (BinaryBasicBlock *BB : F) {
```

This change causes the AArch64 tests to fail, because this function is called (with `EmitColdPart==false`) even when the layout is empty. There are a few options to resolve this: separate the function into two functions, such that one deals with the constant islands and the other one emits the fragments. Then either one calls the other, or you update all callsites. The other option is to wrap the `for (BinaryBasicBlock *BB...` in a conditional. The last option is two return a default/null fragment from `FunctionLayout::getFragment()`, but I'd like to avoid adding those semantics to `FunctionFragment`.

The first solution is probably the correct one, but adds extra noise to this diff. The second solution creates a lot of indenting. Either way, this will eventually be rewritten to support more than two fragments. Unless you think we should change it regardless, I'd suggest we keep iterating over the blocks, for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129518