Page MenuHomePhabricator

treapster (Denis)
User

Projects

User does not belong to any projects.

User Details

User Since
May 18 2022, 1:13 AM (6 w, 2 d)

Recent Activity

Wed, Jun 15

treapster added a comment to D127741: [Disassembler] Add +all CPU feature to subtargetInfo and use it for disassembling AArch64.

For now i'd like to hear more about possible clashing instruction encodings from @labrinea, and if it's not a concern, i think we can fix the tests if we remove CHECK-UNKNOWN: and only call objdump with CHECK-INST. What do you think?

Wed, Jun 15, 12:34 AM · Restricted Project, Restricted Project, Restricted Project
treapster added a comment to D127741: [Disassembler] Add +all CPU feature to subtargetInfo and use it for disassembling AArch64.
Wed, Jun 15, 12:27 AM · Restricted Project, Restricted Project, Restricted Project

Tue, Jun 14

treapster updated the summary of D127741: [Disassembler] Add +all CPU feature to subtargetInfo and use it for disassembling AArch64.
Tue, Jun 14, 7:54 AM · Restricted Project, Restricted Project, Restricted Project
treapster added a comment to D127741: [Disassembler] Add +all CPU feature to subtargetInfo and use it for disassembling AArch64.

Well, it broke a lot of tests that very explicitly checking that instructions are not disassembled without explicitly specified extensions, but i don't quite get the point of such tests. If new instructions were added by default in lldb, why can't we do the same with objdump?

Tue, Jun 14, 7:53 AM · Restricted Project, Restricted Project, Restricted Project
treapster updated the summary of D127741: [Disassembler] Add +all CPU feature to subtargetInfo and use it for disassembling AArch64.
Tue, Jun 14, 7:51 AM · Restricted Project, Restricted Project, Restricted Project
treapster requested review of D127741: [Disassembler] Add +all CPU feature to subtargetInfo and use it for disassembling AArch64.
Tue, Jun 14, 6:32 AM · Restricted Project, Restricted Project, Restricted Project

Fri, Jun 10

treapster updated subscribers of D127220: [BOLT][AArch64] Preserve in text object alignment.

I want to note that the case of openSSL is not as simple as "the object is aligned on 512 and has the size of 512 bytes." The size of iotas function which @rafaelauler referenced above is 192 bytes, and the assumption made by the code is not that it begins at 0x100-aligned address, but that it ends there. Thus, the patch in its current form does not fix the issue. To handle this case we have to estimate island size and then emit it at such address that the end of it has the same alignment as in the input binary.

Fri, Jun 10, 1:43 PM · Restricted Project, Restricted Project

Thu, Jun 9

treapster added a comment to D127111: [BOLT][AArch64] Handle data at the beginning of a function when disassembling and building CFG..

@rafauler, yes, it'll be cool if you commit. My author string is Denis Revunov <revunov.denis@huawei-partners.com>. Thanks in advance!

Thu, Jun 9, 2:18 AM · Restricted Project, Restricted Project

Wed, Jun 8

treapster added a comment to rG907ca2584128: [BOLT-AArch64] Support large test binary.

Thanks for the info, @rafauler! Of course we don't mind a couple of kilobytes of disk space, i was mainly worried about increased working set as you said. There are cases in openssl when one function calls another and they both use some CI, and when it gets duplicated it may push some other data out of cache. And since in the original binary it was working without inlining and without extra instructions, i was thinking about trying to avoid it in the output by keeping CIs/functions together. However, there doesn't seem to be many such cases outside of openssl, and CIs are usually no more than 512 B, so i guess we should just ignore it and solve alignment problems by inlining with original alignment... I'm a bit concerned that inlining with alignment 0x100 and more will create pretty big padding chunks which will inflate functions even more, but since there's not much CI's with such alignment, we don't care too. I guess we'll put this topic aside until someone puts half a megabyte CI in their code and references it from 10+ functions at the same time. Thanks!

Wed, Jun 8, 8:32 AM
treapster added a comment to D127111: [BOLT][AArch64] Handle data at the beginning of a function when disassembling and building CFG..

Please fix the test

Wed, Jun 8, 7:10 AM · Restricted Project, Restricted Project
treapster updated the diff for D127111: [BOLT][AArch64] Handle data at the beginning of a function when disassembling and building CFG..

(Presumably) Fix test

Wed, Jun 8, 7:00 AM · Restricted Project, Restricted Project

Mon, Jun 6

treapster added a comment to rG907ca2584128: [BOLT-AArch64] Support large test binary.

@rafauler, I investigated a bit and discussed the problem with colleagues and now understand that adr immediate offset is only 20 bits, but it should be enough in most cases, especially within hot code. I guess it's possible to handle cases where we can't reach data separately - and not necessarily by inlining, but by using 2-3 instructions to load address or (much harder) localizing functions around CIs they depend on. I now get the idea of the commit, but would still be glad to hear what you think of all that.

Mon, Jun 6, 9:38 AM
treapster added inline comments to D127111: [BOLT][AArch64] Handle data at the beginning of a function when disassembling and building CFG..
Mon, Jun 6, 8:25 AM · Restricted Project, Restricted Project
treapster updated the diff for D127111: [BOLT][AArch64] Handle data at the beginning of a function when disassembling and building CFG..

Add more specific warning

Mon, Jun 6, 8:12 AM · Restricted Project, Restricted Project
treapster updated the diff for D127111: [BOLT][AArch64] Handle data at the beginning of a function when disassembling and building CFG..

Seems like clang used on buildkite doesn't mark .space directive as data, changed test to use repeated .byte instead

Mon, Jun 6, 7:50 AM · Restricted Project, Restricted Project
treapster requested review of D127111: [BOLT][AArch64] Handle data at the beginning of a function when disassembling and building CFG..
Mon, Jun 6, 6:42 AM · Restricted Project, Restricted Project
treapster added a comment to rG907ca2584128: [BOLT-AArch64] Support large test binary.

Hello @rafauler , i was trying to find the source of issues with openssl after optimizing with bolt and discovered that it was caused by the fact that some openssl functions rely on particular constant island alignment - more precisely, on the fact that the end of CI is aligned to some boundary such as 0x100. Inline assembly iterates over islands by pointer, increments it every iteration and uses expressions such as pointerToCI & 0xff == 0 as an ending condition for the loop. Thus, when islands are inlined, the alignment is changed and the loops don't end when they have to. I was trying to find why are CIs inlined and found this commit, but i still don't get why do we have to duplicate CIs in every function that references them. I get that this commit is 5 years old, but would still appreciate any information regarding this decision - that is, what prevents us from referencing the same CI from different functions the same way it was in the input binary? And what can we do to overcome it? In my opinion inlining CIs is just useless inflating of code size even if we didn't have alignment problems with them, and by getting rid of inlining we can both reduce code size and solve alignment problems by just preserving original alignment. Right now if i just remove the code that emits dependent CIs, i get assertion failures at at RuntimeDyldELF::resolveAArch64Relocation and i'll be digging to find where this relocations come from, but i'll be happy to hear your thoughts or advice on the problem.

Mon, Jun 6, 4:31 AM

May 31 2022

treapster added a comment to D126177: [BOLT] [AArch64] Handle constant islands spanning multiple functions.

@rafauler, my author string is Denis Revunov <revunov.denis@huawei-partners.com>. Thanks for your time!

May 31 2022, 12:03 PM · Restricted Project, Restricted Project

May 27 2022

treapster updated the diff for D126177: [BOLT] [AArch64] Handle constant islands spanning multiple functions.

Removed llvm:: qualifications i missed last time

May 27 2022, 2:05 PM · Restricted Project, Restricted Project
treapster updated the diff for D126177: [BOLT] [AArch64] Handle constant islands spanning multiple functions.

Removed unnecessary llvm:: qualification, reworded title and summary.
@rafauler, i don't have commit access so i'll be glad if you commit.

May 27 2022, 1:57 PM · Restricted Project, Restricted Project

May 26 2022

treapster updated the diff for D126177: [BOLT] [AArch64] Handle constant islands spanning multiple functions.

@rafauler, added assembly for test. Regarding getMarkerType, the intent of removing architecture check is not to move the function to SymbolRef, but primarily to decouple it from BinaryContext instance - probably leaving it in BinaryContext.cpp. If you think it fits there as a method, well, then we can leave it as it is.

May 26 2022, 12:54 PM · Restricted Project, Restricted Project
treapster added inline comments to D126177: [BOLT] [AArch64] Handle constant islands spanning multiple functions.
May 26 2022, 3:24 AM · Restricted Project, Restricted Project
treapster added a comment to D126177: [BOLT] [AArch64] Handle constant islands spanning multiple functions.

@rafauler, even though isMarker and getMarkerType fit well to binary context, i wouldn't say we will do unnecessary work on x86 if we extract them, because we check arch at RewriteInstance.cpp:951 anyway. IsMarker is also used one time at BinaryFunction::isSymbolValidInScope, but it's called there anyway, and if we don't want to call it on X86 we can add that check to if. This is largely irrelevant and doesn't make practical difference, but i feel like it's more natural when being or not being a marker is an independent property of a symbol. But these are just my thoughts.

May 26 2022, 2:30 AM · Restricted Project, Restricted Project
treapster updated the diff for D126177: [BOLT] [AArch64] Handle constant islands spanning multiple functions.

Removed isDataMarker/isCodeMarker, fixed test to be independent of shell and linker.

May 26 2022, 2:05 AM · Restricted Project, Restricted Project
treapster added inline comments to D126177: [BOLT] [AArch64] Handle constant islands spanning multiple functions.
May 26 2022, 12:59 AM · Restricted Project, Restricted Project

May 25 2022

treapster updated the diff for D126177: [BOLT] [AArch64] Handle constant islands spanning multiple functions.

Thank you for the feedback, @rafaelauler ! I fixed the test and removed packed attribute as you said.

May 25 2022, 2:17 AM · Restricted Project, Restricted Project
treapster retitled D126177: [BOLT] [AArch64] Handle constant islands spanning multiple functions from [BOLT] Fix disassembly of data in text to [BOLT] [AArch64] Handle data markers spanning multiple functions.
May 25 2022, 1:38 AM · Restricted Project, Restricted Project
treapster changed the edit policy for D126177: [BOLT] [AArch64] Handle constant islands spanning multiple functions.
May 25 2022, 12:16 AM · Restricted Project, Restricted Project

May 24 2022

treapster updated the diff for D126177: [BOLT] [AArch64] Handle constant islands spanning multiple functions.

Remove empty line

May 24 2022, 8:37 AM · Restricted Project, Restricted Project
treapster updated the diff for D126177: [BOLT] [AArch64] Handle constant islands spanning multiple functions.

Removed getFirstInstructionOffset to commit it separately, rebased diff

May 24 2022, 8:30 AM · Restricted Project, Restricted Project

May 22 2022

treapster retitled D126177: [BOLT] [AArch64] Handle constant islands spanning multiple functions from [BOLT] Mark every symbol in $d-$x range as data to [BOLT] Fix disassembly of data in text.
May 22 2022, 2:01 PM · Restricted Project, Restricted Project
treapster requested review of D126177: [BOLT] [AArch64] Handle constant islands spanning multiple functions.
May 22 2022, 1:49 PM · Restricted Project, Restricted Project