[PATCH] D68065: Propeller: LLD Support for Basic Block Sections
Sriraman Tallam via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 25 15:23:56 PST 2020
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:
> 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?
================
Comment at: lld/ELF/Arch/X86_64.cpp:175
+ return (R.type == R_X86_64_PLT32 || R.type == R_X86_64_PC32 ||
+ R.type == R_X86_64_PC8);
+}
----------------
MaskRay wrote:
> tmsriram wrote:
> > MaskRay wrote:
> > > Neither _PC8 nor _PC32 is tested.
> > How do I test this? LLVM uses PLT32 relocs for this. Potentially, PC32 or PC8 can be used for this.
> ```
> .byte 0xe8
> .long foo - . - 4
> ```
Okay, added a test for PC32. Also, the PC8 reloc will be created when we shrink a jmp insn offset from 32 to 8. The actual shrink code will be presented in a separate patch which will test for the PC8 so I will leave that out for now. Further, when we add new psABI relocs, this code and tests will have to be modified.
================
Comment at: lld/ELF/InputSection.cpp:946
for (const Relocation &rel : relocations) {
+ if (rel.expr == R_NONE)
+ continue;
----------------
tmsriram wrote:
> MaskRay wrote:
> > After patching `expr` to R_NONE, propeller should delete these relocations. InputSection.cpp should not know R_NONE has to be skipped.
> I assumed it would be helpful in general to avoid R_NONE relocations here. I dont mind deleting the R_NONE relocations with Propeller.
I am assuming this is alright.
================
Comment at: lld/ELF/OutputSections.cpp:346
+ // Check if this IS needs a special filler.
+ if (isec->SpecialFiller)
+ fill(start, end - start, *(isec->SpecialFiller));
----------------
MaskRay wrote:
> tmsriram wrote:
> > ruiu wrote:
> > > This SpecialFiller is always NOP instructions, so how about adding just a boolean flag (e.g. `isec->fallthrough`) to an input section and remove this `SpecialFiller` vector?
> > Done.
> Is `target->nopInstrs` redundant?
I can just assert instead!
================
Comment at: lld/ELF/Target.h:96
+
+ virtual unsigned growJmpInsn(InputSection &IS, InputFile *File,
+ uint32_t MaxAlignment) const
----------------
tmsriram wrote:
> MaskRay wrote:
> > If growJmpInsn is X86 specific. Can it be moved to Arch/X86_64.cpp?
> growJmpInsn has been removed from this patch and will be presented as a separate patch.
The grow part has been split, not applicable.
================
Comment at: lld/ELF/Writer.cpp:49
+using std::chrono::system_clock;
+using std::chrono::duration;
----------------
MaskRay wrote:
> Delete these.
Not applicable.
================
Comment at: lld/ELF/Writer.cpp:559
// finalizeSections does that.
+ //auto startFinalizeSectionTime = system_clock::now();
finalizeSections();
----------------
MaskRay wrote:
> Delete unneeded comments.
Not applicable.
================
Comment at: lld/ELF/Writer.cpp:644
+ StringRef name = sym.getName();
+
+ if (name.empty() && sym.type == llvm::ELF::STT_NOTYPE &&
----------------
MaskRay wrote:
> Delete the blank line.
Not applicable.
================
Comment at: lld/ELF/Writer.cpp:646
+ if (name.empty() && sym.type == llvm::ELF::STT_NOTYPE &&
+ sym.binding == llvm::ELF::STB_LOCAL) {
+ return false;
----------------
MaskRay wrote:
> Delete surrounding `{}`
Not applicable.
================
Comment at: lld/ELF/Writer.cpp:652
+ lld::propeller::SymbolEntry::isBBSymbol(name)) {
+ if (config->propellerKeepNamedSymbols ||
+ propeller::PropLeg.shouldKeepBBSymbol(name))
----------------
MaskRay wrote:
> return condition
>
> rather than
>
> if condition
> return true
> else
> return false
Not applicable.
================
Comment at: lld/ELF/Writer.cpp:705
ObjFile<ELFT> *f = cast<ObjFile<ELFT>>(file);
+ std::list<Symbol *> localNonBBSymbols;
+ std::list<Symbol *> localBBSymbols;
----------------
MaskRay wrote:
> Use vectors and move them outside the loop.
Not applicable.
================
Comment at: lld/ELF/Writer.cpp:732
+ // Add BB symbols to SymTab first.
+ for (auto *S : localBBSymbols) {
+ in.symTab->addSymbol(S);
----------------
MaskRay wrote:
> Delete {}
> auto -> Symbol
Not applicable.
================
Comment at: lld/ELF/Writer.cpp:1663
+
+ if (Def->value > NewSize) {
+ LLVM_DEBUG(llvm::dbgs() << "Moving symbol " << Sym->getName() <<
----------------
tmsriram wrote:
> PkmX wrote:
> > MaskRay wrote:
> > > This is a dangerous operation. Are you arbitrarily changing st_value/st_size? In what conditions can this be triggered?
> > I think having symbols in the part of section that is shrunk should just be an error, or at least it should clamp them to the end of a section.
> This is called from optimizeBBJumps() and can only be triggered when the size of the section is shrunk from deleting jump instructions.
I will address this in your most recent comment.
================
Comment at: lld/ELF/Writer.cpp:1685
+// relaxation pass does that.
+template <class ELFT> void Writer<ELFT>::optimizeBasicBlockJumps() {
+ if (!config->optimizeBBJumps || !ELFT::Is64Bits)
----------------
MaskRay wrote:
> optimizeBasicBlockJumps should be placed in finalizeAddressDependentContent to accommodate thunks and linker script symbol changes.
Accomodating thunks and creating them optimally needs a little more thought. We haven't looked at this and we haven't tested it.
Could we do this in a later patch?
================
Comment at: lld/ELF/Writer.cpp:1708
+
+ auto MaxIt = config->shrinkJumpsAggressively ? Sections.end() :
+ std::max_element(Sections.begin(), Sections.end(),
----------------
MaskRay wrote:
> MaxIt is not needed.
>
> ```
> MaxAlign = 0;
> if !config->shrinkJumpsAggressively && !sections.empty()
> MaxAlign = max_element(...)->alignment;
> ```
Not applicable.
================
Comment at: lld/ELF/Writer.cpp:1715
+
+ // Shrink jump Instructions optimistically
+ std::vector<unsigned> Shrunk(Sections.size(), 0);
----------------
MaskRay wrote:
> More comments on how sections are shrunk.
Not applicable.
================
Comment at: lld/ELF/Writer.cpp:1732
+ Num += std::count_if(Shrunk.begin(), Shrunk.end(),
+ [] (int e) { return e > 4; });
+ if (Num > 0)
----------------
MaskRay wrote:
> Magic 4 here seems very x86 specific.
Not applicable.
================
Comment at: lld/ELF/Writer.cpp:1738
+ script->assignAddresses();
+ } while (AnyChanged);
+
----------------
PkmX wrote:
> Let the user specify how many rounds to run? I suppose the last few rounds may have marginal benefits.
Not applicable.
================
Comment at: lld/ELF/Writer.cpp:1740
+
+ if (config->shrinkJumpsAggressively) {
+ // Grow jump instructions when necessary
----------------
ruiu wrote:
> Please add a comment as to in what condition this shrink sections too much. If it is monotonically decreasing and alignments are all the same, I think this condition should never occur.
Not applicable.
================
Comment at: lld/ELF/Writer.cpp:1747
+ InputSection &IS = *Sections[I];
+ unsigned BytesGrown = target->growJmpInsn(IS, IS.getFile<ELFT>(), MaxAlign);
+ Changed[I] = (BytesGrown > 0);
----------------
MaskRay wrote:
> ruiu wrote:
> > Is growing different from undoing? I'm not quite sure why you can't simply revert all changes we made to a section if we have to grow it.
> Another related question: why does **shrink**JumpsAggressively call growJmpInsn?
Not applicable.
================
Comment at: lld/ELF/Writer.cpp:1747
+ InputSection &IS = *Sections[I];
+ unsigned BytesGrown = target->growJmpInsn(IS, IS.getFile<ELFT>(), MaxAlign);
+ Changed[I] = (BytesGrown > 0);
----------------
tmsriram wrote:
> MaskRay wrote:
> > ruiu wrote:
> > > Is growing different from undoing? I'm not quite sure why you can't simply revert all changes we made to a section if we have to grow it.
> > Another related question: why does **shrink**JumpsAggressively call growJmpInsn?
> Not applicable.
Not applicable.
================
Comment at: lld/ELF/Writer.cpp:1642
+
+ if (def->value + def->size > NewSize) {
+ LLVM_DEBUG(llvm::dbgs()
----------------
MaskRay wrote:
> Similarly, in BFD, this is:
>
> else if (sym->st_value <= addr
> && sym->st_value + sym->st_size > addr
> && sym->st_value + sym->st_size <= toaddr)
> sym->st_size -= count;
>
Working on this one, will fix this in the next iteration. Keeping this open until then.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68065/new/
https://reviews.llvm.org/D68065
More information about the llvm-commits
mailing list