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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Some random comments...



================
Comment at: lld/ELF/Arch/X86_64.cpp:154
+        IS.relocations[I].expr != R_NONE)
+      break;
+  }
----------------
nit: `Return I` here and add an `llvm_unreachable` after the loop, so that we can catch an impossible condition at runtime.


================
Comment at: lld/ELF/Arch/X86_64.cpp:191-192
+
+  // If this jmp is a fall thru, the target offset is the beginning of the
+  // next section.
+  uint64_t NextSectionOffset = NextIS->getOutputSection()->addr +
----------------
If you define new set of relocation types for bb sections, you can make it guaranteed that the relocations always point to the beginning of a section (because if there's a jump instruction jumping to a middle of a bb section, it is not really a BB).


================
Comment at: lld/ELF/OutputSections.cpp:346
+      // Check if this IS needs a special filler.
+      if (isec->SpecialFiller)
+        fill(start, end - start, *(isec->SpecialFiller));
----------------
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?


================
Comment at: lld/ELF/Writer.cpp:1740
+
+    if (config->shrinkJumpsAggressively) {
+      // Grow jump instructions when necessary
----------------
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.


================
Comment at: lld/ELF/Writer.cpp:1747
+          InputSection &IS = *Sections[I];
+          unsigned BytesGrown = target->growJmpInsn(IS, IS.getFile<ELFT>(), MaxAlign);
+          Changed[I] = (BytesGrown > 0);
----------------
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.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D68065