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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Arch/X86_64.cpp:171
+// next section.
+// TODO: Delete this once a new relocation is added for this.
+static bool isFallThruRelocation(InputSection &is, InputFile *file,
----------------
Delete this once psABI reservers a new relocation type for fallthrough jumps.


================
Comment at: lld/ELF/Arch/X86_64.cpp:181
+                                                      addrLoc, *r.sym, r.expr),
+                   (config->wordsize * 8));
+
----------------
ruiu wrote:
> nit: remove extra parentheses
Delete `SignExtend64`.

config->wordsize * 8 is a constant, 64.


================
Comment at: lld/ELF/Arch/X86_64.cpp:185
+  // next section.
+  uint64_t NextSectionOffset =
+      nextIS->getOutputSection()->addr + nextIS->outSecOff;
----------------
Fix variable case.


================
Comment at: lld/ELF/Arch/X86_64.cpp:187
+      nextIS->getOutputSection()->addr + nextIS->outSecOff;
+  return ((addrLoc + 4 + targetOffset) == NextSectionOffset);
+}
----------------
Delete excess parentheses


================
Comment at: lld/ELF/Arch/X86_64.cpp:258
+  // To flip, there must be atleast one JmpCC and one direct jmp.
+  if (is.getSize() < (sizeOfDirectJmpInsn + sizeOfJmpCCInsn))
+    return 0;
----------------
Delete excess parentheses


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


================
Comment at: lld/ELF/Writer.cpp:1633
+
+      if (def->value > NewSize) {
+        LLVM_DEBUG(llvm::dbgs()
----------------
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;



================
Comment at: lld/ELF/Writer.cpp:1642
+
+      if (def->value + def->size > NewSize) {
+        LLVM_DEBUG(llvm::dbgs()
----------------
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;



================
Comment at: lld/ELF/Writer.cpp:1677
+      InputSection *next =
+          (i + 1) < sections.size() ? sections[i + 1] : nullptr;
+      InputSection &is = *sections[i];
----------------
excess parentheses


================
Comment at: lld/test/ELF/bb-sections-and-icf.s:22
+.section	.text.bar,"ax", at progbits
+# -- Begin function bar
+.type	bar, at function
----------------
This comment is redundant.


================
Comment at: lld/test/ELF/bb-sections-and-icf.s:37
+.section	.text.foo,"ax", at progbits
+# -- Begin function foo
+.type	foo, at function
----------------
ditto


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

https://reviews.llvm.org/D68065