This is an archive of the discontinued LLVM Phabricator instance.

[BOLT][Golang support] Core changes
Needs ReviewPublic

Authored by yota9 on Jan 8 2023, 1:42 PM.

Details

Summary

This is part of BOLT core changes for golang support patch: https://reviews.llvm.org/D124347

Diff Detail

Event Timeline

yota9 created this revision.Jan 8 2023, 1:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 8 2023, 1:42 PM
yota9 requested review of this revision.Jan 8 2023, 1:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 8 2023, 1:42 PM
yota9 edited the summary of this revision. (Show Details)Jan 9 2023, 5:24 AM
yota9 updated this revision to Diff 492429.Jan 26 2023, 6:27 AM

Small changes

yota9 added a comment.Jan 28 2023, 6:05 AM

I'm not sure what causes clang-format to complain about BF.h , locally I'm unable to repeat this.
Anyway I've commented about 3 main problems in this patch. I would be happy to discuss them and submit the golang support patches one-by-one, it would be the most right and simple away I think.
@maksfb @rafauler @Amir

bolt/lib/Core/BinaryBasicBlock.cpp
638

We need this, since some of the instructions annotations might be "locked". E.g. when dealing with golang inlined functions, we might inline only one instruction (e.g NOP) which will be removed by bolt. But this instruction has inline metadata that we need to have in order to restore inlining golang tables. So before removing such instruction we need to create NOP instruction on it's place that will store this metadata.

bolt/lib/Core/BinaryContext.cpp
2001

Golang pass needs to create 2 extra read sections. We need to create PHDR for them and emit them properly to the binary. I'm not sure how to refactor the code connected to extra section handling, would be thankful for ideas.

bolt/lib/Core/BinarySection.cpp
169

This is currently used for the created by golang pass sections. They don't have input data, so it is used as a crutch where we mark the section as changed and use its output contents for emission. With the new sections creating/emission that is probable the second problem we need to discuss.
P.S. The golang pass mmaps the memory with overhead since during section creation we're unable to table the final size. It is not a problem to use malloc/new, I've done it so the memory would be allocated lazily on the page access, but actually the sizes there are not so big..

bolt/include/bolt/Core/BinaryContext.h
237

Do you care about the order? Or is a SmallStringSet sufficient?

yota9 marked an inline comment as done.Jan 28 2023, 6:19 AM
yota9 added inline comments.
bolt/include/bolt/Core/BinaryContext.h
237

Basically you are right here, but this is temp code and one of the problem that was marked by me above - we need to think about refactoring the code that emits extra sections, so probably it would be removed.

Elvina added a subscriber: Elvina.Jan 31 2023, 6:19 AM
yota9 marked an inline comment as done.Feb 6 2023, 5:20 AM
yota9 added a subscriber: rafaelauler.

New writable segment creation was re-developed and separate review was created, let's start from there https://reviews.llvm.org/D143390 @maksfb @rafaelauler @Amir

yota9 updated this revision to Diff 495600.Feb 7 2023, 10:57 AM

Change patch according to https://reviews.llvm.org/D143390
@maksfb @rafauler please review D143390 :)