[PATCH] D51089: [LLD] [COFF] Add support for creating range extension thunks for ARM

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: COFF/Chunks.cpp:50
+// indicates.
+void SectionChunk::finalizeContents() {
+  if (!RelocTargets.empty())
----------------
ruiu wrote:
> We generally don't micro-optimize code, but for relocations, we do, because the number of relocations can be an order of tens of millions for large programs. Spending one more microsecond for each relocation adds up to one second if your program has one million relocations. This function is a bit concerning in that regard. Could you measure the performance impact?
> 
> Also, it looks odd that you do this in `finalizeContents`, as it doesn't correspond to finalizing contents. Perhaps this function should be given a new name.
I tried measuring it, and I think it's making things slower, but it's mostly within measurement noise.

My testcase was linking a 66 MB clang.exe (for x86_64). Before this change, the fastest link was in 480 ms, after the change the fastest link was 520 ms. But in both cases the runtimes occasionally go up to over 600 ms. So it's not huge but I think it's consistently measurable.

Do you have any other suggestions on how to achieve this without affecting performance? Only use the RelocTargets vector if `Machine==ARMNT`? Or make it a `DenseMap` for overridden targets, which is empty for all other cases than when we have added thunks?

Yes, it's a bit odd with `finalizeContents`, maybe a new method `initRelocTargets` which just gets called once before we start doing `assignAddresses`?


================
Comment at: COFF/Chunks.h:369-372
+  Defined *getTarget() const { return Target; }
+
+private:
+  Defined *Target;
----------------
ruiu wrote:
> We generally don't define trivial accessors like this; instead, just define a member as a public one.
Ok, will include that change into the next update.


https://reviews.llvm.org/D51089