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

Tue Mar 24 17:30:21 PDT 2020

MaskRay added a comment.

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/

> 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 it because of concerns about long-term viability and maintenance.

================
Comment at: lld/ELF/Arch/X86_64.cpp:159
+    if (is.relocations[i].offset == offset && is.relocations[i].expr != R_NONE)
+      break;
+  }
----------------
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;
}
```

================
Comment at: lld/ELF/Arch/X86_64.cpp:66
+// which are executed during fall through.
+static const std::vector<std::vector<uint8_t>> X86_NOP_INSTRUCTIONS = {
+    {0x90},
----------------
Should follow `variableName` convention.

Add `See X86AsmBackend::writeNopData`

================
Comment at: lld/ELF/Arch/X86_64.cpp:171
+static bool isRelocationForJmpInsn(Relocation &R) {
+  return (R.type == R_X86_64_PLT32 || R.type == R_X86_64_PC32 ||
+          R.type == R_X86_64_PC8);
----------------
delete parens

================
Comment at: lld/ELF/Arch/X86_64.cpp:254
+  // relocation is 4 bytes before the end.
+  unsigned rIndex = getRelocationWithOffset(is, (is.getSize() - 4));
+  if (rIndex == is.relocations.size())
----------------
Nit: delete parens `(is.getSize() - 4)`

================
Comment at: lld/ELF/Options.td:47
+    "Remove direct jumps at the end to the next basic block",
+    "Do not remove any direct jumps at the end to the next basic block">;
+
----------------
Add `(default)`. See other options.

================
Comment at: lld/ELF/Options.td:512
+    "Give unique names to every basic block section for LTO",
+    "Do not give unique names to every basic block section for LTO">;
 def shuffle_sections: J<"shuffle-sections=">, MetaVarName<"<seed>">,
----------------
`"Do not give unique names to every basic block section for LTO (default)"`

================
Comment at: lld/ELF/OutputSections.cpp:260
+  if (nopFiller[remaining - 1].size() != remaining)
+    fatal("failed padding with special filler");
+  memcpy(buf + i, nopFiller[remaining - 1].data(), remaining);
----------------
`assert(nopFiller[remaining - 1].size() == remaining)`

================
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) {
----------------
The code (`if (isec->nopFiller)`) self explains. No need for a comment.

================
Comment at: lld/ELF/Target.h:96

+  //  This deletes a jump insn at the end of the section if it is a fall thru to
+  //  the next section.  Further, if there is a conditional jump and a direct
----------------
delete excess space after `// `

================
Comment at: lld/ELF/Writer.cpp:2118
+  // sections.
+  //auto startOptBBJumpTime = system_clock::now();
+  optimizeBasicBlockJumps();
----------------
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?

================
Comment at: lld/ELF/Writer.cpp:1682
+        LLVM_DEBUG(llvm::dbgs()
+                   << "Moving symbol " << Sym->getName() << " from "
+                   << def->value << " to "
----------------
Nit: Moving -> move (make it a bit simpler)

================
Comment at: lld/ELF/Writer.cpp:1706
+template <class ELFT> void Writer<ELFT>::optimizeBasicBlockJumps() {
+  if (!config->optimizeBBJumps)
+    return;
----------------
Nit: move the condition to the call site.

================
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
----------------
There is only one list item. Why use `1.` ?

================
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
----------------
Where is `Step 2`?

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

================
Comment at: lld/ELF/Writer.cpp:1729
+      result[i] =
+          target->deleteFallThruJmpInsn(is, is.getFile<ELFT>(), next) ? 1 : 0;
+    });
----------------
Nit: static_cast<unsigned>

================
Comment at: lld/ELF/Writer.cpp:1735
+      LLVM_DEBUG(llvm::dbgs()
+                 << "Removing " << numDeleted << " fall through jumps\n");
+    }
----------------
Removing -> removed

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

================
Comment at: lld/test/ELF/bb-sections-and-icf.s:4
+## This simple test checks foo is folded into bar with bb sections
+## and the jumps are deletd.
+
----------------
deleted

================
Comment at: lld/test/ELF/bb-sections-and-icf.s:6
+
+# 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
----------------
`x86_64-pc-linux` -> `x86_64`

================
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
----------------
delete excess space.

Prefer `--optimize-bb-jumps` over `-optimize-bb-jumps`

================
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:
> delete excess space.
> 
> Prefer `--optimize-bb-jumps` over `-optimize-bb-jumps`
If --icf=all result is different from `--icf=none`. Add a comment.

================
Comment at: lld/test/ELF/bb-sections-and-icf.s:16
+# CHECK:	<a.BB.foo>
+# Explicity check that bar is folded and not emitted.
+# CHECK-NOT:    <bar>
----------------
Use `## ` for test comments.

================
Comment at: lld/test/ELF/bb-sections-and-icf.s:17
+# Explicity check that bar is folded and not emitted.
+# CHECK-NOT:    <bar>
+# CHECK-NOT:    <a.BB.bar>
----------------
Don't add excess space.

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

https://reviews.llvm.org/D68065