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

Wed Feb 26 15:42:25 PST 2020

MaskRay added a comment.

A Phabricator tip: marking every review comment `Not applicable.` is not needed. You can just click "Close" (status: Unsubmitted) but don't click `Submit`. When you upload a new Diff (with `arc diff`), the comments will be closed automatically.

I believe a large number of `Not applicable` comments were originally applicable. There have been many changes (see the number of `Diff` in History: 16). A large number of comments were not addressed with several versions. Now a bunch of comments were commented as `Not applicable` at once... It made it slightly difficult to understand which Diff addresses the comments. Though, they don't matter now.

When you upload a new Diff, I hope you can rebase the patch on origin/master. "Patch not applicable on master" has been complained by other reviewers in some other patches.

I think a patch hierarchy has been suggested by other reviewers and me several times. It is not clear what patches to apply and in what order. You can mention it in the `SUMMARY`. It will be very useful and also save reviewers' time. (To be honest I felt a lot of pressure when I received messages after I clicked "Request Changes". I try to be responsive.)

================
Comment at: lld/ELF/Arch/X86_64.cpp:159
+    if (is.relocations[i].offset == offset && is.relocations[i].expr != R_NONE)
+      break;
+  }
----------------
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.

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

https://reviews.llvm.org/D68065