Page MenuHomePhabricator

espindola (Rafael Avila de Espindola)
User

Projects

User does not belong to any projects.

User Details

User Since
Dec 30 2017, 7:39 PM (49 w, 4 d)

Recent Activity

Apr 27 2018

espindola added inline comments to D46214: Avoid reading past end of archive looking for long file name.
Apr 27 2018, 5:23 PM
espindola added a comment to D45571: [ELF] - Speedup MergeInputSection::splitStrings.

The test results are interesting

Apr 27 2018, 2:58 PM
espindola added a comment to D45571: [ELF] - Speedup MergeInputSection::splitStrings.

You have to update

Apr 27 2018, 1:52 PM
espindola added a comment to D45571: [ELF] - Speedup MergeInputSection::splitStrings.

I think this is fine. I will run the benchmarks locally to confirm.

Apr 27 2018, 1:51 PM
espindola added inline comments to D46145: Use a buffer when allocating relocations.
Apr 27 2018, 1:45 PM
espindola added a comment to D46205: Set MemoryBuffer's RequiresNullTerminator to false by default..

I really like the idea of changing the default, but maybe instead of an argument we should just have two functions:

Apr 27 2018, 1:27 PM
espindola closed D46208: Don't create a temporary DenseMap for each input .eh_frame.

331075

Apr 27 2018, 1:24 PM
espindola retitled D46208: Don't create a temporary DenseMap for each input .eh_frame from Don't create a temporary CieMap for each input .eh_frame to Don't create a temporary DenseMap for each input .eh_frame.
Apr 27 2018, 1:07 PM
espindola created D46208: Don't create a temporary DenseMap for each input .eh_frame.
Apr 27 2018, 1:07 PM
espindola created D46207: Avoid some memory allocations in the ThreadPool.
Apr 27 2018, 12:49 PM
espindola closed D46196: Split .eh_frame sections in parellel.

331064

Apr 27 2018, 11:21 AM
espindola added inline comments to D45850: [ELF] Read the call graph profile from object files..
Apr 27 2018, 10:27 AM
espindola added a comment to D44965: [llvm][Instrumentation/MC] Add Call Graph Profile pass and object file emission..

The MC and Object parts are enough for lld, yes.

Apr 27 2018, 10:23 AM
espindola updated the diff for D46145: Use a buffer when allocating relocations.

Delete call to reserve.

Apr 27 2018, 10:06 AM
espindola added a comment to D46145: Use a buffer when allocating relocations.

I tried this patch and found that at least for Chrome, we wasted memory by calling reserve() on ".data.rel.ro" sections. Such section in Chrome doesn't add any item to Sec.Relocations while their Rels.size() is relatively large, so reserved memory is totally wasted. If you don't call reserve on such section, you can save almost the same memory as you did in this patch. Can you take a look?

Apr 27 2018, 9:58 AM
espindola created D46196: Split .eh_frame sections in parellel.
Apr 27 2018, 9:46 AM
espindola closed D46169: Split merge sections early.

r331058

Apr 27 2018, 9:35 AM
espindola added a comment to D46169: Split merge sections early.

I think it is probably OK. What about adding an assert checking that we do decompress only for non-alloc sections?

Apr 27 2018, 8:44 AM
espindola updated the diff for D46145: Use a buffer when allocating relocations.

correct patch .

Apr 27 2018, 8:37 AM
espindola added a comment to D46145: Use a buffer when allocating relocations.

Then maybe we should remove it?

Apr 27 2018, 8:34 AM
espindola updated the diff for D46145: Use a buffer when allocating relocations.

Use end() when inserting. Doesn't make a difference in here, but is the canonical way of concatenating vectors.

Apr 27 2018, 8:32 AM
espindola added a comment to D46145: Use a buffer when allocating relocations.

I wonder if you can make a guess on how many relocations will be inserted to the vector. We know the number of relocations for each input section, so calling reserve() might work.

I think reserve() is exactly what causes high peaks now. We already call it. See:
https://github.com/llvm-mirror/lld/blob/master/ELF/Relocations.cpp#L1020

Apr 27 2018, 8:28 AM
espindola added inline comments to D46145: Use a buffer when allocating relocations.
Apr 27 2018, 8:09 AM
espindola added a comment to D46145: Use a buffer when allocating relocations.
Apr 27 2018, 7:44 AM

Apr 26 2018

espindola updated the summary of D46169: Split merge sections early.
Apr 26 2018, 8:41 PM
espindola closed D46038: Also demote lazy symbols.
Apr 26 2018, 8:36 PM
espindola created D46169: Split merge sections early.
Apr 26 2018, 8:05 PM
espindola added a comment to D46163: [DO NOT SUBMIT] Do strlen() while computing xxhash.

It is possible that xxhash is just too slow for use in a hash table. The experiment I did for pr37029 using hash_combine was still using strlen.

Apr 26 2018, 7:26 PM
espindola added inline comments to D45571: [ELF] - Speedup MergeInputSection::splitStrings.
Apr 26 2018, 7:19 PM
espindola added a comment to D46145: Use a buffer when allocating relocations.

Does your code make this part of code faster? It reduces memory allocation but it does extra memcpy, so I'm wondering.

Apr 26 2018, 3:45 PM
espindola added a comment to D44965: [llvm][Instrumentation/MC] Add Call Graph Profile pass and object file emission..

The part MC and Object part of this patch LGTM.

Apr 26 2018, 3:42 PM
espindola added a comment to D46145: Use a buffer when allocating relocations.

This seems a bit tricky. Doesn't shrink_to_fit work?

Apr 26 2018, 3:18 PM
espindola accepted D44560: [DWARF] Rework debug line parsing to use llvm::Error and callbacks.

Since this is mainly about debug info and I not an expert in the area, please get one more LGTM before committing.

Apr 26 2018, 2:56 PM
espindola accepted D46099: [ELF] Clarify help wording for --symbol-ordering-file.

The new message is IMO slightly better, so I am OK with that patch.

But I am actually thinking about something like:
"Layout sections to place symbols in the order specified by symbol ordering file"

Apr 26 2018, 2:47 PM
espindola added a comment to D46099: [ELF] Clarify help wording for --symbol-ordering-file.

Yeah, that's also true. Slightly unrelated to this change, but --warn-symbol-ordering is silent right now in case you forget -ffunction-sections (but of course the ordering doesn't work). Would it be reasonable to extend the warnings to cover that case? I'm thinking, for each symbol specified in the symbol ordering file, warn if that symbol's section also contains other symbols, though I don't know if that would lead to false positives and/or be reasonable cost-wise.

Maybe we can warn if two or more symbols specified by a symbol ordering file belong to one section? I'm not sure if that would cause false positives, but it might be worth to try.

I agree that an additional warning here might be nice. We would probably only want to warn once (or possibly some other low limit) per section, not per symbol, as otherwise if you forget -ffunction-sections, you will get far too many warnings.

Apr 26 2018, 2:47 PM
espindola updated the summary of D46145: Use a buffer when allocating relocations.
Apr 26 2018, 2:42 PM
espindola created D46145: Use a buffer when allocating relocations.
Apr 26 2018, 2:39 PM
espindola closed D46103: Replace SharedSymbols with Defined when creating copy relocations.

330966

Apr 26 2018, 11:04 AM
espindola updated the diff for D46103: Replace SharedSymbols with Defined when creating copy relocations.

Implement the remaining suggestions.

Apr 26 2018, 10:56 AM
espindola added inline comments to D46103: Replace SharedSymbols with Defined when creating copy relocations.
Apr 26 2018, 10:52 AM
espindola updated the diff for D46103: Replace SharedSymbols with Defined when creating copy relocations.

Rebased and address some of the review comments.

Apr 26 2018, 10:48 AM
espindola closed D46094: Simplify processRelocAux.

r330960

Apr 26 2018, 10:27 AM

Apr 25 2018

espindola updated the diff for D46104: Add a Global symbol type.

Fix a crash.

Apr 25 2018, 11:37 PM
espindola updated the summary of D46104: Add a Global symbol type.
Apr 25 2018, 10:28 PM
espindola created D46104: Add a Global symbol type.
Apr 25 2018, 10:28 PM
espindola created D46103: Replace SharedSymbols with Defined when creating copy relocations.
Apr 25 2018, 10:19 PM
espindola created D46094: Simplify processRelocAux.
Apr 25 2018, 5:03 PM
espindola accepted D46087: Remove unused features from StringRefZ and move it to Symbols.h..

LGTM

Apr 25 2018, 3:27 PM
espindola closed D46080: Pack symbols a bit more.

330874

Apr 25 2018, 2:49 PM
espindola updated the diff for D46080: Pack symbols a bit more.

Use uint32_t

Apr 25 2018, 2:33 PM
espindola added a comment to D46080: Pack symbols a bit more.

Maybe we should remove StringRefZ class and use a char pointer instead? The point of having StringRefZ class is that an instance of StringRefZ is automatically converted to StringRefZ when needed, but I think we no longer use the feature after this patch.

Apr 25 2018, 2:32 PM
espindola created D46080: Pack symbols a bit more.
Apr 25 2018, 1:56 PM
espindola added a comment to D46038: Also demote lazy symbols.

r330869.

Apr 25 2018, 1:50 PM

Apr 24 2018

espindola created D46038: Also demote lazy symbols.
Apr 24 2018, 6:42 PM
espindola added a comment to D46036: Bring r329960 back.

r330788.

Apr 24 2018, 5:35 PM
espindola closed D46036: Bring r329960 back.

330788

Apr 24 2018, 5:35 PM
espindola added a reviewer for D46036: Bring r329960 back: pcc.
Apr 24 2018, 5:14 PM
espindola added a comment to D46036: Bring r329960 back.

Actually, I misread the location of the call to demoteSharedSymbols. I now think this is the proper fix as it just simplifies the symbols before we start processing the relocations.

Apr 24 2018, 5:14 PM
espindola created D46036: Bring r329960 back.
Apr 24 2018, 4:58 PM
espindola added a comment to D45536: Do not keep shared symbols to garbage-collected eliminated DSOs..

I am finally trying to reduce what went wrong with this patch.

Apr 24 2018, 2:39 PM
espindola closed D45365: [bugpoint] Fix crash when testing for miscompilation.

330763

Apr 24 2018, 1:35 PM
espindola accepted D45434: [ELF] - Eliminate AssertCommand..

LGTM

Apr 24 2018, 11:35 AM
espindola added a comment to D45164: [MC] Change AsmParser to leverage Assembler during evaluation.

Since this is using information inside a single fragment when producing assembly I am OK with it.

Apr 24 2018, 11:33 AM
espindola added inline comments to D45571: [ELF] - Speedup MergeInputSection::splitStrings.
Apr 24 2018, 11:08 AM

Apr 23 2018

espindola edited reviewers for D45942: [Support/Path] Make handling of paths like "///" consistent, added: Bigcheese; removed: espindola.
Apr 23 2018, 9:12 PM
espindola accepted D45516: [ELF] - Refactor lazy symbol duplicated code..

LGTM

Apr 23 2018, 8:31 PM
espindola accepted D45969: [ELF] - Never use std::sort..

LGTM

Apr 23 2018, 8:23 PM

Apr 20 2018

espindola accepted D45902: Add "-z lazy" and "-z relro"..

LGTM

Apr 20 2018, 2:23 PM
espindola accepted D45849: [ELF] --warn-backrefs: use the same GroupId for object files in the same --{start,end}-lib.

LGTM

Apr 20 2018, 8:03 AM

Apr 19 2018

espindola added a comment to D45571: [ELF] - Speedup MergeInputSection::splitStrings.

I believe a fast strlen() can be implemented using SSE instructions. And if you are using SSE instructions, your data is loaded to XMM registers. I believe there exists a fast vectorized hash function that works on data on a XMM register. I wonder if we can combine the two to create a single fast function that returns the length of a string as well as its hash value.

Apr 19 2018, 6:46 PM
espindola added a comment to D45849: [ELF] --warn-backrefs: use the same GroupId for object files in the same --{start,end}-lib.

Does --start-lib/--end-lib really implies --start-group/--end-group?

Apr 19 2018, 6:40 PM
espindola updated the diff for D45399: Split SectionPiece in its parts.

Have the OutputOffsets store just a uint64_t. This uses a bit of the hash for the live bit.

Apr 19 2018, 6:37 PM
espindola closed D45830: Fix trap instruction on pp64.

r330386

Apr 19 2018, 6:25 PM
espindola created D45830: Fix trap instruction on pp64.
Apr 19 2018, 11:48 AM

Apr 18 2018

espindola created D45791: Cache getSymVA.
Apr 18 2018, 6:20 PM

Apr 16 2018

espindola added a comment to D45642: [PPC64] V2 abi: Add glink section for lazy symbol resolution..

This is a plt by another name, no?

Do you know why it is defined to have another name?

I'm not sure what you mean by 'plt' here. In this abi the .plt section is just the array of addresses of external functions that the dynamic linker fills out at runtime. The .glink section holds the lazy resolution stubs which will setup the environment for the dynamic linker to do so. Then there are also stubs for calling the external functions by loading their address out of the .plt. Am I right to assume by 'plt' you mean a combination of lazy resolver and the call stub?

Apr 16 2018, 7:23 PM
espindola accepted D36351: [lld][ELF] Add profile guided section layout.

LGTM with a few last requests.

Apr 16 2018, 5:45 PM · lld
espindola added a comment to D45536: Do not keep shared symbols to garbage-collected eliminated DSOs..
In D45536#1069314, @rnk wrote:

This is causing LLD to drop __cxa_finalize from the symbol tables of some Chromium binaries, and that causes them to crash on shutdown. See the test failures here:
https://ci.chromium.org/buildbot/chromium.clang/ToTLinux/2152

I'm going to revert this for now. I found that re-linking just libipc_mojom_shared.so and libmojo_mojom_bindings_shared.so with LLD after this change causes ipc_tests to crash reliably on shutdown.

Apr 16 2018, 4:45 PM
espindola added a comment to D45642: [PPC64] V2 abi: Add glink section for lazy symbol resolution..

This is a plt by another name, no?

Apr 16 2018, 11:07 AM
espindola added a comment to D45571: [ELF] - Speedup MergeInputSection::splitStrings.

It should also be possible to template Hash.h over the returned type so that some clients can explicitly request a 32 or 64 bit hash. Not sure if that change would be accepted.

Do you know why it have to use size_t, btw? Given that hash_value falls back to short_hash that returns uint64_t, I wonder if it was intentional design or can be changed to always use uint64_t.

Apr 16 2018, 10:58 AM
espindola accepted D45548: Avoid hash table lookup when sorting local symbols..

LGTM with the sort predicate fixed.

Apr 16 2018, 9:45 AM
espindola added inline comments to D45516: [ELF] - Refactor lazy symbol duplicated code..
Apr 16 2018, 9:28 AM

Apr 13 2018

espindola added a comment to D45571: [ELF] - Speedup MergeInputSection::splitStrings.

No much difference for me:

Function Name                                   Total CPU(%) Total CPU (ms)

* After the change:
- lld.exe (PID: 15032)                          100.00        4166
 + lld::elf::MergeInputSection::splitIntoPieces  22.40         933

* Default (xxHash64):
- lld.exe                                         100.00%        4254
 + lld::elf::MergeInputSection::splitStrings       21.86%         930
Apr 13 2018, 11:18 AM
espindola added inline comments to D36351: [lld][ELF] Add profile guided section layout.
Apr 13 2018, 10:24 AM · lld
espindola created D45630: Avoid having a map for edges.
Apr 13 2018, 10:24 AM
espindola added inline comments to D36351: [lld][ELF] Add profile guided section layout.
Apr 13 2018, 9:15 AM · lld

Apr 12 2018

espindola added inline comments to D36351: [lld][ELF] Add profile guided section layout.
Apr 12 2018, 9:32 PM · lld
espindola created D45607: Store initial cluster weight instead of normalized edge weights.
Apr 12 2018, 9:31 PM
espindola abandoned D45577: Remove the Section structure.
Apr 12 2018, 9:15 PM
espindola accepted D45536: Do not keep shared symbols to garbage-collected eliminated DSOs..

LGTM with a small change.

Apr 12 2018, 2:36 PM
espindola added a comment to D45548: Avoid hash table lookup when sorting local symbols..

The results I got

Apr 12 2018, 2:17 PM
espindola added inline comments to D45548: Avoid hash table lookup when sorting local symbols..
Apr 12 2018, 2:13 PM
espindola added a comment to D45548: Avoid hash table lookup when sorting local symbols..

My idea was actually to combine both patches.

Apr 12 2018, 1:30 PM
espindola added a comment to D45571: [ELF] - Speedup MergeInputSection::splitStrings.

I just noticed that hash_short will read at most 64 bytes of the string.

Apr 12 2018, 1:09 PM
espindola added inline comments to D45571: [ELF] - Speedup MergeInputSection::splitStrings.
Apr 12 2018, 11:21 AM
espindola added a comment to D45571: [ELF] - Speedup MergeInputSection::splitStrings.

Interesting. The patch by itself seems fine. I will benchmark it locally.

Apr 12 2018, 11:18 AM
espindola added a comment to D45375: [ELF] - Introduce synthetic file for linker script symbols..

Would never having a null InputFile be sufficient?

Like assigning a dummy ObjFile or something to synthetic symbols?

Apr 12 2018, 10:46 AM
espindola added inline comments to D36351: [lld][ELF] Add profile guided section layout.
Apr 12 2018, 10:16 AM · lld
espindola created D45577: Remove the Section structure.
Apr 12 2018, 10:06 AM
espindola added inline comments to D36351: [lld][ELF] Add profile guided section layout.
Apr 12 2018, 9:29 AM · lld