[PATCH] D68065: Propeller: LLD Support for Basic Block Sections
Sriraman Tallam via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 25 20:35:12 PDT 2020
tmsriram added a subscriber: amharc.
tmsriram added a comment.
In D68065#1940343 <https://reviews.llvm.org/D68065#1940343>, @MaskRay wrote:
> Hi Sriraman,
>
> Sounds like there is strong support for your patch, so let’s move forward on it.
>
> I do have a few more code review items I’d like addressed if we can before we commit.
> There are several nits about excess parentheses which do not fit with the rest of lld code.
> Please scan the full patch and delete them. Rebase and upload a new diff so that I can patch my local repository with `arc patch D68065`.
> It does not apply so it isn’t convenient for me to review the tests now.
>
> In addition, we probably should move test/ELF/bb-sections-* tests to test/ELF/propeller/
In D68063 <https://reviews.llvm.org/D68063> and D73674 <https://reviews.llvm.org/D73674>, we decided (one of your suggestions too) to remove all references to Propeller as this was related to basic block sections. This is also fully a part of bb sections and fits naturally and is part of core LLD. I dont think we should move it to propeller/.
>
>
>> I don't mean any disrespect here but your tone suggests that you are quite experienced in this area :) If you have a better proposal, I strongly encourage you to propose it, evaluate it with experiments and performance numbers and get it into LLVM. Asking us to evaluate completely new designs along the lines of Full or Partial LTO is not feasible as it takes several weeks if not months, and IMHO, not reasonable particularly at this stage in the review.
>
> The first sentence is fair. I have just a bit more than 2 years experience contributing to LLVM. I made a llvm-dev proposal as everyone can see. In my spare time I will probably make more investigations how to speed up things.
>
>> We have put in a lot of effort towards this work to come this far. Asking us to go do it totally differently is not something we are going to do. We now know the partial LTO idea was actually not yours and only suggested to you, which I believe you should at least acknowledge for transparency.
>
> Just wanted to defend for myself. I was not sure I could mention the person. That email also includes lots of my own ideas.
>
>> You also say in your previous message that "I am very glad to see that we have made progress by landing D68063 <https://reviews.llvm.org/D68063> ..." and yet you are blocking this. This is ridiculous.
>
> Please note that I made it very clear that basic block sections are helpful for other things. I was blocking this linker patch because of concerns about long-term viability and maintenance.
>
> That said, I think we need to try out new ideas. I am now focusing on code view itself, instead of repeating my previous high level concerns.
================
Comment at: lld/ELF/Arch/X86_64.cpp:159
+ if (is.relocations[i].offset == offset && is.relocations[i].expr != R_NONE)
+ break;
+ }
----------------
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.
================
Comment at: lld/ELF/OutputSections.cpp:352
end = buf + sections[i + 1]->outSecOff;
- fill(start, end - start, filler);
+ // Check if this IS needs a special filler.
+ if (isec->nopFiller) {
----------------
MaskRay wrote:
> The code (`if (isec->nopFiller)`) self explains. No need for a comment.
I removed it but maybe I should say that NOPs are needed here as opposed to TRAP because they might be executed.
================
Comment at: lld/ELF/Writer.cpp:2118
+ // sections.
+ //auto startOptBBJumpTime = system_clock::now();
+ optimizeBasicBlockJumps();
----------------
MaskRay wrote:
> MaskRay wrote:
> > Delete unrelated comments.
> `optimizeBasicBlockJumps` calls assignAddresses, which was only called in finalizeAddressDependentContent.
>
> We hope assignAddresses caller are grouped together (if in.symTab needs to be finalized first, please add a comment). Can you move this pass immediately before (or after) finalizeAddressDependentContent?
Correct me if I am wrong but finalizing in.symtab before we optimize seems important, I added a comment here. If we relax jumps going forward, we would definitely need to know how far they are.
================
Comment at: lld/ELF/Writer.cpp:1682
+ LLVM_DEBUG(llvm::dbgs()
+ << "Moving symbol " << Sym->getName() << " from "
+ << def->value << " to "
----------------
MaskRay wrote:
> Nit: Moving -> move (make it a bit simpler)
I am not a native speaker but "Moving" seems more appropriate here unless you meant that 'M' should be lower case. My take, "move" seems to imply the user must do it.
================
Comment at: lld/ELF/Writer.cpp:1706
+template <class ELFT> void Writer<ELFT>::optimizeBasicBlockJumps() {
+ if (!config->optimizeBBJumps)
+ return;
----------------
MaskRay wrote:
> Nit: move the condition to the call site.
Moved and asserted at the beginning.
================
Comment at: lld/ELF/Writer.cpp:1711
+ // For every output section that has executable input sections, this
+ // does this:
+ // 1. It deletes all direct jump instructions in input sections that
----------------
MaskRay wrote:
> There is only one list item. Why use `1.` ?
Rephrased, shrinking the jump instrs was the 2. but I split that patch out and this remained unnoticed.
================
Comment at: lld/ELF/Writer.cpp:1721
+ std::vector<unsigned> result(sections.size());
+ // Step 1: Delete all fall through jump instructions. Also, check if two
+ // consecutive jump instructions can be flipped so that a fall through jmp
----------------
MaskRay wrote:
> Where is `Step 2`?
Same deal.
================
Comment at: lld/ELF/Writer.cpp:1726
+ InputSection *next =
+ (i + 1) < sections.size() ? sections[i + 1] : nullptr;
+ InputSection &is = *sections[i];
----------------
MaskRay wrote:
> Nit: delete parens
With parens reads better to me personally. If this is not acceptable w.r.t the coding style, lmk and I will delete.
================
Comment at: lld/ELF/Writer.cpp:1735
+ LLVM_DEBUG(llvm::dbgs()
+ << "Removing " << numDeleted << " fall through jumps\n");
+ }
----------------
MaskRay wrote:
> Removing -> removed
Past tense is preferred? I see mixed use here in many places in LLD.
================
Comment at: lld/ELF/Writer.cpp:1633
+
+ if (def->value > NewSize) {
+ LLVM_DEBUG(llvm::dbgs()
----------------
MaskRay wrote:
> MaskRay wrote:
> > Better to check if there are cases where st_value > original_input_section_size.
> >
> > I asked because in binutils, bfd/elfnn-riscv.c has a similar check.
> >
> > if (sym->st_value > addr && sym->st_value <= toaddr)
> > sym->st_value -= count;
> >
> I requested a research for st_value but I think it is not needed to get it accurate.
>
> However, I don't think we should just copy the elfnn-riscv.c behavior.
>
> if (def->value + def->size > NewSize && def->value <= OldSize && def->value + def->size <= OldSize) {
>
> should be simplified to
>
> ```
> if (def->value + def->size > NewSize && def->value + def->size <= OldSize) {
> ```
@amharc We are going back and forth here. We didn't think it was necessary to copy the behavior either, but since you suggested that we copy elfnn-riscv.c behavior, we went ahead and did it. We dont see a problem with either, so should we just leave it as is? Is there a particular concern here, that is not clear to me.
================
Comment at: lld/test/ELF/bb-sections-and-icf.s:7
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: ld.lld -optimize-bb-jumps --icf=all %t.o -o %t.out
+# RUN: llvm-objdump -d %t.out| FileCheck %s --check-prefix=CHECK
----------------
MaskRay wrote:
> MaskRay wrote:
> > delete excess space.
> >
> > Prefer `--optimize-bb-jumps` over `-optimize-bb-jumps`
> If --icf=all result is different from `--icf=none`. Add a comment.
Didn't follow. This test explicitly checks for the folding. You want to test icf=none too? Why?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68065/new/
https://reviews.llvm.org/D68065
More information about the llvm-commits
mailing list