- User Since
- Feb 14 2018, 11:09 AM (234 w, 23 h)
I tested on x86-64 ubuntu 22 and confirmed it builds.
Thanks Simon, I think this patch makes a nice improvement to the behavior of objdump. I'm not the code owner of these disassembly tools, but the patch looks good to me. I would rather wait for the input of whoever is more familiar with objdump, though.
Mon, Aug 8
Fri, Aug 5
Wed, Aug 3
Fri, Jul 29
Thu, Jul 28
Can we add more testing that exercises the new code paths being added here?
Wed, Jul 27
Fri, Jul 22
I'm also curious if this adds runtime overhead by parsing LSDA two times per function. Can we measure that using a large binary as input?
Excellent work Huan! I have a few design suggestions below. Let me know what do you think.
Wed, Jul 20
Those should be available in your build folder, here:
Fri, Jul 15
Wed, Jul 13
Tue, Jul 12
I agree, erasing functions in that pass seems hacky. Thanks.
Jul 11 2022
Jul 8 2022
There's a memory leak the two tests:
Thanks for working on improving hugify!
Looks good, let me kick off tests
I think that works. Feel free to reopen or create a new review, whichever is easier. CI runs:
Jul 6 2022
The cause of the crash caused by this diff is because this diff moves processing of interprocedural references outside the disassembly loop.
Jul 5 2022
Jun 28 2022
Hi @yota9, I reverted this commit because it is causing BOLT to crash in one of your internal binaries, let me work a bit more on it to understand what is happening before you land it again. Thanks!
Jun 27 2022
Jun 24 2022
Rename typo in title BOTL -> BOLT
Thanks Huan for all the work put in this diff. I have a few more comments below.
That's awesome, thanks
Jun 23 2022
Do you have a testcase?
FYI if you just want to test, I think you can publish this as "draft" using "arc diff --draft"
Jun 21 2022
It's a bit confusing to call a function named "stable_sort", which is a well-known C++ stdlib function, but we're actually calling stable_sort from LLVM and not from stdlib. I've tried looking up in LLVM repo and there are all sorts of usages of is_contained, for example: some of them use llvm:is_contained, others use is_contained directly. What is your take on this? Should we call llvm::stable_sort, or just stable_sort?
Jun 17 2022
Jun 15 2022
Jun 10 2022
TBH it's not like code that makes weird layout assumptions is really important/buying us much performance anyway... so we might just replace it if we can., at the source, or just skip it / preserve it untouched in the original section.
The way we support openSSL users for x86 (or users of any assembly-written libs that have layout assumptions, for that matter) is usually via -skip-funcs.
Jun 9 2022
Jun 8 2022
For reference in this discussion, offending code is here: https://github.com/openssl/openssl/blob/master/crypto/sha/asm/keccak1600-armv8.pl#L87
Thanks @yota9 for working on this! I have some questions below.
Jun 7 2022
Jun 6 2022
Hi @treapster. The purpose of emitting data in the middle of the code section in RISC machines is to have it close to code that refers to it, so as to use less instructions to address this data. Otherwise, you would do it x86-way and just emit all data in the data sections, which could be far away (depends on how large is your text section). It's possible to argue that it's better for the cache hierarchy to emit data far away, and that for x86, since there is no penalty to address far-away data, it's a bad idea to emit data in the middle of the text section. The reason would be to compact code close together to use less pages in iTLB -- here, having non-executable bytes in .text pages is just wasted space from the point of view of i-cache.
Jun 3 2022
Btw, the stashed changes can't be recovered by any of the commands listed in your print statement, right? You would need to run git stash apply
Conceptually, the problem is that we need a full cross-compiler to build to a specific target (linux), but we only have the compiler, not the libs. That's why I thought it would make sense to restrict to environments where we are not cross compiling to a different system. Looking at "pie.test", for example, we are apparently building a .cpp file, which is even worse, as it requires libstdc++. Somehow I want to give test writers the idea that it is not OK to build C++ files unless you are not cross-compiling.
(I'm talking about a separate test repo, btw. The in-tree test should still be fixed individually for each diff to avoid breaking bisecting tools)
We should time it to land in the same day and then land changes to tests that compensates for the aggregate of all changes. Regarding the order, I don't have any preference, as long as both patchsets hit the repo in the same day.
Btw from BOLT perspective this diff LGTM. Regarding the changes to X86 target, those look minimal and rather benign to me. It is apparently using tablegen info (GET_INSTRINFO_OPERAND_TYPE) that is not used by any other backend that is in-tree, but TableGen supports generating this info and it is even in their testsuite. Even if it has a bug because it hasn't been used, I think it makes sense to try for us as it obviously won't break any existing code as we are the only users of this information. We should wait for someone responsible for the X86 target to approve this change, though.
Let me try to elaborate on why this diff is important and has functional changes.
No, this diff is not NFC. It's actually pretty impactful (and thanks for working on this Amir).
Jun 2 2022
You're right that it does result in functional changes.
@spupyrev I don't remember if you have commit access to the LLVM repo. If you don't, let me know, and I'll commit this once you are done with modifications and give me a green signal.
Jun 1 2022
Thanks! LGTM pending the test fix
May 31 2022
Okay, looks good to me