This is an archive of the discontinued LLVM Phabricator instance.

[BOLT] Add function layout class
ClosedPublic

Authored by FPar on Jul 11 2022, 2:37 PM.

Details

Summary

This patch adds a dedicated class to keep track of each function's
layout. It also lays the groundwork for splitting functions into
multiple fragments (as opposed to a strict hot/cold split).

Diff Detail

Event Timeline

FPar created this revision.Jul 11 2022, 2:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2022, 2:37 PM
FPar requested review of this revision.Jul 11 2022, 2:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2022, 2:37 PM
yota9 added inline comments.Jul 12 2022, 5:45 AM
bolt/include/bolt/Core/BinaryBasicBlock.h
678

Hello, I didn't investigate this patch carefully for now, but why FragmentNum needs to be unsigned? It seems it is used like previously used isCold flag.

FPar marked an inline comment as done.Jul 12 2022, 8:00 AM
FPar added inline comments.
bolt/include/bolt/Core/BinaryBasicBlock.h
678

It is unsigned, because it will be used to reference the fragment in which the basic block resides. You are correct that as of this patch there is only a hot and cold fragment and FragmentNum only assumes values 0 or 1. This will change with subsequent patches, in which support for multiple fragment splitting will be introduced.

For this patch, I'll change the definition of getFragmentNum() to depend on IsCold, and retain the original definition for isCold()/setIsCold().

FPar updated this revision to Diff 444050.Jul 12 2022, 12:36 PM
FPar marked an inline comment as done.

Split diff to reduce noise from replacing references to function layout to
block list.

FPar updated this revision to Diff 444145.Jul 12 2022, 9:02 PM

Rebase on previous commit

tschuett added inline comments.
bolt/include/bolt/Core/FunctionLayout.h
43

If you use an enum class, then you can get rid of the prefixes.

FPar marked an inline comment as done.Jul 13 2022, 6:37 AM
FPar added inline comments.
bolt/include/bolt/Core/FunctionLayout.h
43

This enum is to be used temporarily to pass into APIs that take a fragment number as parameter for parts of BOLT that are only hot/cold aware (so we can perform a more incremental transition from relying on .blocks() to being fully multi-fragment aware). The code using those values will be nicer if they automatically convert to unsigned. Thanks for looking over this!

FPar updated this revision to Diff 444339.Jul 13 2022, 10:56 AM
FPar marked an inline comment as done.

Move FunctionFragment out of FunctionLayout class

Just as a comment. Maybe down the line, you could replace the unsigned with a FragmentId, a wrapper around an unsigned, to avoid confusion.

I have a feeling that you want to deprecate FunctionLayout::blocks(). There is probably a LLVM_DEPRECATED macro.

FPar updated this revision to Diff 444436.Jul 13 2022, 3:22 PM

Wrap FragmentNum in new type

FPar added a comment.Jul 13 2022, 3:30 PM

Just as a comment. Maybe down the line, you could replace the unsigned with a FragmentId, a wrapper around an unsigned, to avoid confusion.

I have a feeling that you want to deprecate FunctionLayout::blocks(). There is probably a LLVM_DEPRECATED macro.

Your intution is correct, we want to get rid of FunctionLayout::blocks(). We discussed marking it as deprecated, but we figured it'll mostly create warning noise for now, because it is used too pervasively. So we defer that for now.

I did add a type for FragmentNum. I want to avoid the term "Id" here, because it is not used as an identifier across functions and can change arbitrarily in case if fragments are inserted in the middle of a function. Thanks for the suggestion!

maksfb added inline comments.Jul 14 2022, 1:53 PM
bolt/include/bolt/Core/BinaryFunction.h
1378–1379

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.

bolt/include/bolt/Core/FunctionLayout.h
80
177
bolt/lib/Core/BinaryEmitter.cpp
401

We can take advantage of the new layout class and iterate over fragments here.

bolt/lib/Core/BinaryFunction.cpp
3221
3554
FPar updated this revision to Diff 444805.Jul 14 2022, 2:56 PM
FPar marked 6 inline comments as done.

Add isSplit() method to FunctionLayout + fix nits

bolt/include/bolt/Core/BinaryFunction.h
1378–1379

I agree.

bolt/lib/Core/BinaryEmitter.cpp
401

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.

rafauler added inline comments.Jul 15 2022, 4:15 PM
bolt/lib/Core/FunctionLayout.cpp
105

monotonically? unless the blocks are boring

maksfb added inline comments.Jul 15 2022, 4:51 PM
bolt/include/bolt/Core/FunctionLayout.h
12–15
62

Will the reference work?

maksfb accepted this revision.Jul 15 2022, 5:34 PM

LGTM

bolt/include/bolt/Core/BinaryFunction.h
2335

Same as block_front() (same for the next three occurrences)?

bolt/include/bolt/Core/FunctionLayout.h
82
149
This revision is now accepted and ready to land.Jul 15 2022, 5:34 PM
FPar updated this revision to Diff 445261.Jul 16 2022, 2:06 PM
FPar marked 6 inline comments as done.

Address feedback

This revision was automatically updated to reflect the committed changes.