[PATCH] D68065: Propeller: LLD Support for Basic Block Sections

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Arch/X86_64.cpp:159
+    if (is.relocations[i].offset == offset && is.relocations[i].expr != R_NONE)
+      break;
+  }
----------------
tmsriram wrote:
> tmsriram wrote:
> > tmsriram wrote:
> > > MaskRay wrote:
> > > > tmsriram wrote:
> > > > > MaskRay wrote:
> > > > > > MaskRay wrote:
> > > > > > > tmsriram wrote:
> > > > > > > > MaskRay wrote:
> > > > > > > > > In an old comment, I suggested
> > > > > > > > > 
> > > > > > > > > > See Relocations.cpp:scanRelocs. A better way is to sort relocations beforehand.
> > > > > > > > > 
> > > > > > > > > It is not addressed.
> > > > > > > > Could you please reconsider this? I understand what you mean here.
> > > > > > > > This code is going to change when new psABI relocations are added.  Could we re-open this then? 
> > > > > > > What is the average size of `is.relocations.size()`? It it is small in practice, the comment should mention it.
> > > > > > ```
> > > > > > for (unsigned i = is.relocations.size(); i != 0; ) {
> > > > > >   --i;
> > > > > >   if (is.relocations[i].offset == offset && is.relocations[i].expr != R_NONE)
> > > > > >     return i;
> > > > > > }
> > > > > > ```
> > > > > This won't work because this function should return the max. size when not found.  If I change that assumption, I must also change how this is consumed by its callers. Is there a reason why you want to search in the reverse direction?  I mean I will change it if  you could tell me why.
> > > > ```
> > > > for (unsigned i = is.relocations.size(); i != 0; ) {
> > > >   --i;
> > > >   if (is.relocations[i].offset == offset && is.relocations[i].expr != R_NONE)
> > > >     return i;
> > > > }
> > > > return is.relocations.size();
> > > > ```
> > > > 
> > > > An input section may have several relocations. Scanning backward can be more efficient.
> > > We discussed this f2f and even about sorting it and you expressed that you were alright leaving it as is, maybe you forgot.  
> > Forgot to add that since this will change when the new relocations come in.
> Alright, backward scanning now.
No, I do not forget it. The relocations are still sorted in practice. It is just that this is not a guaranteed property. Iterating the relocations backward is faster.


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

https://reviews.llvm.org/D68065