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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/ELF/Arch/X86_64.cpp:90
   trapInstr = {0xcc, 0xcc, 0xcc, 0xcc}; // 0xcc = INT3
+  sizedNOPInstrs = X86_NOP_INSTRUCTIONS;
 
----------------
Maybe `nopInstrs` is slightly better?


================
Comment at: lld/ELF/Arch/X86_64.cpp:117-118
+// returns the corresponding enum value.
+static JmpInsnOpcode getJmpInsnType(const uint8_t *First,
+                                    const uint8_t *Second) {
+  if (*Second == 0xe9)
----------------
First -> first, Second -> second


================
Comment at: lld/ELF/Arch/X86_64.cpp:154-155
+// Returns the maximum size of the vector if no such relocation is found.
+static unsigned getRelocationWithOffset(const InputSection &IS,
+                                        uint64_t Offset) {
+  unsigned I = 0;
----------------
IS -> is,Offset -> offset


================
Comment at: lld/ELF/Arch/X86_64.cpp:169
+
+static bool isDirectJmpInsnOpcode(const uint8_t *Opcode) {
+  return (*Opcode == 0xe9);
----------------
Does "direct jmp" actually mean just J_JMP_32?


================
Comment at: lld/ELF/Arch/X86_64.cpp:191-194
+  if ((AddrLoc + 4 + TargetOffset) != NextSectionOffset)
+    return false;
+
+  return true;
----------------
I think you can just return the result of the boolean expression.


================
Comment at: lld/ELF/Arch/X86_64.cpp:224
+  }
+  return J_UNKNOWN;
+}
----------------
You have a `default` as well as this `return` statement, but I guess that compilers are smart enough to figure out that this return is unreachable?


================
Comment at: lld/ELF/Arch/X86_64.cpp:227
+
+// Deletes direct jump instruction in input sections that jumps to the
+// following section as it is not required.  If there are two consecutive jump
----------------
In this context, does "direct jump" mean all J* instructions?


================
Comment at: lld/ELF/Arch/X86_64.cpp:230-231
+// instructions, it checks if they can be flipped and one can be deleted.
+bool X86_64::deleteFallThruJmpInsn(InputSection &IS, InputFile *File,
+                                   InputSection *NextIS) const {
+  const unsigned SizeOfDirectJmpInsn = 5;
----------------
is, file and nextIS


================
Comment at: lld/ELF/Arch/X86_64.cpp:232
+                                   InputSection *NextIS) const {
+  const unsigned SizeOfDirectJmpInsn = 5;
+
----------------
Ditto


================
Comment at: lld/ELF/Arch/X86_64.cpp:276
+  const uint8_t *JmpInsnB = SecContents + Rb.offset - 1;
+  JmpInsnOpcode JO_B = getJmpInsnType(JmpInsnB - 1, JmpInsnB);
+  if (JO_B == J_UNKNOWN)
----------------
Please fix local variable names.


================
Comment at: lld/ELF/InputSection.h:131-133
+  unsigned BytesDropped = 0;
+
+  bool Trimmed = false;
----------------
bytesDropped, trimmed


================
Comment at: lld/ELF/InputSection.h:131-133
+  unsigned BytesDropped = 0;
+
+  bool Trimmed = false;
----------------
ruiu wrote:
> bytesDropped, trimmed
This needs a brief comment as to what they are for, e.g. "If Basic-Block Sections is enabled, most code sections ends with a jump instruction, and if a basic block just fall through in the final code layout, we want to trim the trailing jump instruction by shrinking a section. We have a few members to support that operation."


================
Comment at: lld/ELF/InputSection.h:217-223
+  bool NOPFiller = false;
+
+  // These are modifiers to jump instructions that are necessary when basic
+  // block sections are enabled.  Basic block sections creates opportunities to
+  // relax jump instructions at basic block boundaries after reordering the
+  // basic blocks.
+  std::vector<JumpInstrMod> JumpInstrMods;
----------------
lowercase


================
Comment at: lld/ELF/InputSection.h:225
+
+  void addJumpInstrMod(JumpInstrMod J) { JumpInstrMods.push_back(J); }
+
----------------
Since JumpInstrMod is a public member, you don't need this?


================
Comment at: lld/ELF/LTO.cpp:82
+  if (!config->ltoBBSections.empty()) {
+    if (config->ltoBBSections.equals("all"))
+      c.Options.BBSections = BasicBlockSection::All;
----------------
nit: it is more common to use `operator==` instead of `equals` to compare StringRefs.


================
Comment at: lld/ELF/OutputSections.cpp:246
 
+static void fill(uint8_t *Buf, size_t Size,
+                 const std::vector<std::vector<uint8_t>> &SFiller) {
----------------
Please give this function a new name, as it is not easy to distinguish this function from the other `fill`.


================
Comment at: lld/ELF/OutputSections.cpp:249
+  unsigned I = 0;
+  unsigned NC = Size / SFiller.back().size();
+  for (unsigned C = 0; C < NC; ++C) {
----------------
I think you can directly use `target->sizedNOPInstrs` instead of passing it as an argument.


================
Comment at: lld/ELF/OutputSections.cpp:352
+      if (isec->NOPFiller && target->sizedNOPInstrs)
+        fill(start, end - start, *(target->sizedNOPInstrs));
+      else
----------------
nit: *(target->sizedNOPInstrs) -> *target->sizedNOPInstrs


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

https://reviews.llvm.org/D68065