[PATCH] D115993: [ELF] Optimize RelocationSection<ELFT>::writeTo
Alexander Richardson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 21 09:46:28 PST 2021
arichardson added a comment.
This looks like a nice improvement to me. I also like @ikudrin's suggestion of validating internal consistency in debug builds, but if this has a measurable performance impact the current patch LGTM.
================
Comment at: lld/ELF/SyntheticSections.cpp:1668
+ addend = computeAddend();
+ kind = AddendOnly; // Catch errors
}
----------------
MaskRay wrote:
> ikudrin wrote:
> > And now we have an inconsistency if `kind` was `AddendOnlyWithTargetVA` or `AgainstSymbolWithTargetVA` because `sym` is not `nullptr` and an `assert()` in `computeAddend()` can trigger. Maybe add a new kind `PostComputeRaw` and add `assert`s to `computeAddend()` and `needsDynSymIndex()` to ensure that they are not called after `computeRaw()`?
> Every redundant operation costs. We have comprehensive tests so I do not worry much about inconsistency caused problems. Adding `PostComputeRaw` seems to sacrifice too much for the check.
Not sure if that makes any difference in a release build since we already have the store in computeRaw().
One thing that might also make it more resilient to bugs introduced while refactoring (or to help modified downstream consumers with less comprehensive test coverage) would be to keep the `addend` field private and add a `r_addend()` accessor that asserts that we are in the `PostComputeRaw`/`ContentsFinalized`/`SomeOtherName` state.
================
Comment at: lld/ELF/SyntheticSections.cpp:1775
Elf_Rela r;
- encodeDynamicReloc<ELFT>(getPartition().dynSymTab, &r, rel);
+ r.r_offset = rel.getOffset();
+ r.setSymbolAndType(rel.getSymIndex(getPartition().dynSymTab), rel.type,
----------------
Does it make sense to also parallelize this or is this not meaningful for the relocation counts encountered in practise?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D115993/new/
https://reviews.llvm.org/D115993
More information about the llvm-commits
mailing list