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

[Sriraman Tallam via Phabricator via llvm-commits]

tmsriram marked an inline comment as done.
tmsriram 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;
+  }
----------------
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.  


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

https://reviews.llvm.org/D68065