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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Overall looking good, but it looks like this test file doesn't cover all relocations you want to handle. You are handling the following relocations, and compared to that the test file seems too small.

  J_JMP_32,
  J_JNE_32,
  J_JE_32,
  J_JG_32,
  J_JGE_32,
  J_JB_32,
  J_JBE_32,
  J_JL_32,
  J_JLE_32,
  J_JA_32,
  J_JAE_32,



================
Comment at: lld/ELF/Arch/X86_64.cpp:157
+  unsigned i = 0;
+  for (; i < is.relocations.size(); ++i) {
+    if (is.relocations[i].offset == offset && is.relocations[i].expr != R_NONE)
----------------
nit: you should cache the result of is.relocations.size() as `for (unsigned e = is.relocations.size(); i < e; ++i)`


================
Comment at: lld/ELF/Arch/X86_64.cpp:269
+  const uint8_t *jmpInsnB = secContents + rB.offset - 1;
+  JmpInsnOpcode jmpOpcode_B = getJmpInsnType(jmpInsnB - 1, jmpInsnB);
+  if (jmpOpcode_B == J_UNKNOWN)
----------------
nit: jmpOpcode_B -> jmpOpcodeB


================
Comment at: lld/ELF/Driver.cpp:936
   config->ltoSampleProfile = args.getLastArgValue(OPT_lto_sample_profile);
+  config->ltoBBSections = args.getLastArgValue(OPT_lto_basicblock_sections);
+  config->ltoUniqueBBSectionNames =
----------------
This should be named `ltoBasicblockSections` to exactly match the option name.


================
Comment at: lld/ELF/InputSection.cpp:1021-1022
+    for (const JumpInstrMod &jumpMod : jumpInstrMods) {
+      uint64_t offset = jumpMod.Offset;
+      offset += sec->outSecOff;
+      uint8_t *bufLoc = buf + offset;
----------------
nit: this can be `uint64_t offset = jumpMod.Offset + sec->outSecOff`


================
Comment at: lld/ELF/InputSection.h:134
+  // instruction. The members below help trimming the trailing jump instruction
+  // and shrinking a section.
+  unsigned bytesDropped = 0;
----------------
tmsriram wrote:
> tmsriram wrote:
> > ruiu wrote:
> > > Now I wonder if you can just shrink `rawData`? Then you can revert he change you made to `getSize()`.
> > I looked at this and saw that "trimmed" can be deleted which simplifies the code here and in getSize() much more.  
> > 
> > I need to keep track of actual available space and shrunk space.  This is because we both shrink and grow the section, a follow-up patch that does this optimization.  So, bytesDropped is useful to undo the shrink.  PTAL and see if the simplification helps.
> > 
> > Also, if we dont keep track of the actual size of the section when growing and shrinking, it is not possible to catch bugs where we accidentally grow more than the original size of the section, potentially writing out-of-bounds into rawData.
> Sorry, I mistyped the last line.  It should read "potentially reading out-of-bounds from rawData".  
I see. Maybe we can have two ArrayRefs, one is an ArrayRef of the original size and the other is a (possibly) shrunk one, but I think it doesn't matter much, so I'm fine with this approach.


================
Comment at: lld/ELF/OutputSections.cpp:246
 
+static void nopInstrFill(uint8_t *Buf, size_t Size) {
+  unsigned i = 0;
----------------
Lowercase


================
Comment at: lld/ELF/OutputSections.cpp:250
+  unsigned num = Size / nopFiller.back().size();
+  for (unsigned C = 0; C < num; ++C) {
+    memcpy(Buf + i, nopFiller.back().data(), nopFiller.back().size());
----------------
Lowercase


================
Comment at: lld/ELF/Relocations.h:115-117
+  JumpModType Original;
+  uint64_t Offset;
+  unsigned Size;
----------------
Original -> original
Offset -> offset
Size -> size


================
Comment at: lld/ELF/Target.h:91-92
 
+  virtual void applyJumpInstrMod(uint8_t *Loc, JumpModType Type,
+                                 JumpModType Val) const {}
+
----------------
Lowercase


================
Comment at: lld/ELF/Target.h:101-102
+  //  instruction can be deleted.
+  virtual bool deleteFallThruJmpInsn(InputSection &IS, InputFile *File,
+                                     InputSection *NextIS) const {
+    return false;
----------------
Lowercase


================
Comment at: lld/ELF/Writer.cpp:1657
+template <class ELFT> void Writer<ELFT>::optimizeBasicBlockJumps() {
+  if (!config->optimizeBBJumps || !ELFT::Is64Bits)
+    return;
----------------
What this `!ELFT::Is64Bits` condition for? It looks like you can just remove it.


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

https://reviews.llvm.org/D68065