[PATCH] D103539: WIP: try to repair RISCV handling of paired relocations

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:396
     return;
+
   MCContext &Ctx = Asm.getContext();
----------------
Please don't "fix" unrelated whitespace


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp:56
   const MCExpr *Expr = Fixup.getValue();
+
   // Determine the type of the relocation
----------------
Ditto


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp:156-161
+      std::advance(iterator, RI);
+
+      Relocs[RI].Type = ELF::R_RISCV_ADD8;
+      Relocs.emplace(iterator, Relocs[RI].Offset, nullptr, ELF::R_RISCV_SUB8, 0,
+                     nullptr, 0);
+
----------------
I'm confused why this doesn't overwrite RI again with an R_RISCV_SUB8?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp:160
+      Relocs.emplace(iterator, Relocs[RI].Offset, nullptr, ELF::R_RISCV_SUB8, 0,
+                     nullptr, 0);
+
----------------
This approach is also going to give quadratic complexity. Is there a reason we can't emplace_back? If so, can a stable sort post-processing step not fix that more efficiently (asymptotically)?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp:163
+      ++RI, ++RE;
+      break;
+    }
----------------
Please no comma operators outside of `for`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103539