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

[Sriraman Tallam via Phabricator via llvm-commits]

tmsriram added a comment.

In D68065#1894573 <https://reviews.llvm.org/D68065#1894573>, @MaskRay wrote:

> 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.


Alright, will do that in future.

> 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.

Just to be clear, the original patch also had shrink and grow optimization which we split out of the main patch and hence many of the original comments don't apply.

> 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.

Sure, will do.

> 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.)
> 
>> ! In D68065#1894573 <https://reviews.llvm.org/D68065#1894573>, @MaskRay wrote:
> 
> 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.

Sure, this patch depends on TargetOptions.h changes in D68063 <https://reviews.llvm.org/D68063>. I marked it as a parent patch.  I will put a larger summary of all patches associated with BBSections and Propeller.

> 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.)




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

https://reviews.llvm.org/D68065