[PATCH] D78954: [LLD][ELF] Add isDebug flag to SectionBase.

Tue Apr 28 01:34:51 PDT 2020

grimar added a comment.

In D78954#2006503 <https://reviews.llvm.org/D78954#2006503>, @avl wrote:

> > Most links don't use --strip-debug or --strip-all, so isDebugSection in LinkerDriver::link is rarely called.
>
>
>
> > Most links don't specify -r or --emit-relocs, so InputSection::copyRelocations is not called. In addition, isDebugInfo inside the function is called by edge cases which we have to suppress warnings for practical reasons, the cases are even fewer.
>
>
>
> > I am hesitant to accept this patch, unless you can make further justification.
>
> This change costs nothing. It does not increase the size of SectionBase. Thus it seems to me it would be useful to do this patch even if it does not show noticeable performance impact right now.

I added a comment about this previously, I think it was not answered?
https://reviews.llvm.org/D74169#inline-709439

My assumption was that it is a premature unproved optimization (while you did not show the numbers, e.g. link time with/without it).
(Honestly I would be surprised to see a speed-up after doing this. At least while you have cases with 889% link time, my feeling is that something else is slow, but not this).

> This change costs nothing.

This part is a bit wrong atm. It costs us:

1. A bit in `SectionBase`, which might be needed in the future.
2. It causes code readers to think that this is a useful optimization, though it is unproved yet. (General rule of LLD is do no work that can be avoided. It helps it to be that fast).
3. More code for perhaps no reason.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78954/new/

https://reviews.llvm.org/D78954