[PATCH] D88830: [LLD][ELF] Improve ICF for relocations to ineligible sections via "aliases"

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lld/ELF/ICF.cpp:228
     for (size_t i = begin; i < mid; ++i)
-      sections[i]->eqClass[next] = mid;
+      sections[i]->eqClass[next] = eqClass;
 
----------------
andrewng wrote:
> MaskRay wrote:
> > andrewng wrote:
> > > MaskRay wrote:
> > > > Inline the variable `eqClass`
> > > > 
> > > Just to clarify, you want me to replace `eqClass` with the expression `eqClassBase + mid`, i.e. bring the addition inside the `for` loop?
> > Yes. This is optimizable by the compiler
> OK, no problem, although I have to admit this goes against my instinctive coding approach. But I guess that might be because most projects with which I've been involved, have put at least some importance towards the run-time performance of unoptimized debug builds. But if this is the "LLVM" way, then so be it.
> OK, no problem, although I have to admit this goes against my instinctive coding approach. 

FWIW I agree with it. I also prefer to precalculate values outside of the loop, it looks and reads cleaner usually.

> But if this is the "LLVM" way, then so be it.

I don't think it is, I guess the idea was to get rid of a "excessive variable" to make the code a bit shorter.
I have no strong opinion about this particular place, because it is a very short/simple loop, though personally I would not do it too.


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

https://reviews.llvm.org/D88830