- User Since
- Jul 22 2016, 1:13 PM (182 w, 3 d)
I'm not seeing any changes to the output of llvm-readobj's relocation symbol info (We still only print the symbol name). Are we planning to do it in a follow up patch instead?
Thu, Jan 16
Addressed David's comment.
- Added const.
- Make the assertions useful.
Other than some minor nits. LGTM.
Wed, Jan 15
Mon, Jan 13
Address some minor concerns Hubert have in the offline discussion:
- Get symbol table index using SymA directly, if we can't find it in the symbol table, then find its csect in the symbol table.
- return pairs using uniform init directly in the switch.
Ping on author. Any plan to move this forward? Should it sit in "Plan Changes" queue.
Fri, Jan 10
Thu, Jan 9
Wed, Jan 8
Addressed first round of comments.
Tue, Jan 7
Mon, Jan 6
Fri, Jan 3
Mon, Dec 30
Dec 19 2019
Dec 18 2019
Dec 17 2019
Dec 16 2019
Dec 5 2019
I agree with David about splitting. There are 4 issues mentioned in this patch, and they are not related. (I'm not sure if 1 or 3 could be combine or not)
Putting them together makes it hard to review, and hard to determine if the test case actually covered the issue that's raised.
Let's make separate patches if possible.
LGTM with minor nit.
Dec 4 2019
Dec 3 2019
Address David and Digger's comment.
Nov 28 2019
Fixed incorrect TOC-base alignment.
Nov 27 2019
Minor nit that could be address while landing.
Nov 26 2019
Addressed post review comments in https://github.com/llvm/llvm-project/commit/7707d8aa9db8aa3814593f9c40cc707f306e3ae2
Nov 25 2019
Nov 22 2019
Nov 21 2019
Added -verify-machine-instr and -mcpu for consistency.
Added raw data testing.
Nov 20 2019
Removed unnecessary assertion.
Nov 19 2019
For comparison, gcc generates the value of constant pool data directly into the TC entry. It's faster and garbage collectable if not used.
I did some investigation about LLVM constant pool, as I remember, except for int value which will be put directly into TC entry, others like double, float etc. will be put into constant pool in LLVM. If we want to mimic GCC behavior, it probably would be a large piece of refactoring.
I don't think llvm actually try to put int directly into TC entry, it always put a label in TC entry, or we just use that integer value directly in the assembly. But please correct me if I'm wrong.
We don't need to switch to that behavior right now, but later when we care about performance then we might want to think about it.
Here's my test case:
Nov 18 2019
Some observation for constant pool lowering:
We put all the constant pool data into rodata section, and create one TC entry per constant pool reference.
It hinders the ability for linker to garbage collect unused data. (Generates them into a separate RO section helps a bit, but won't fully solve this issue)
And we would also create a lot of TC entries for constant pool that might not get cleanup.
Sorry, please ignore the test did not come clean comments. I realized my branch did not have D70182 in it. After pulling it in, the test works as expected.
I will continue my review.
The "make check-all" did not return clean for the test case you just added.
There is mismatch for the test result and assertions. You might be missing something in the code that you modified.
Test case is missing when the change is landed. Please add it back.
I believe I addressed all the comments. Thanks.
Added separate CsectGroup to contain function descriptor csects.
Added assertion to assert uniqueness TOC base.
Nov 15 2019
When I apply your patch to master, it didn't apply cleanly. So please rebase.
And when I did some work to manually apply it, and run through clang-format, clang-format changed several places.
I suspect it's due to the tabs vs space issue.
I suggest to update this patch with after running the clang-format on it.
I'm not able to apply this patch cleanly because rebase. Thank you.
Nov 14 2019
Nov 5 2019
Oct 30 2019
Oct 28 2019
This patch is going to have conflict with D67125. Depending on which patch is going to go land first. The test case is likely need to get modified.
Oct 26 2019
Change SectionCount type to unsigned.