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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added a comment.

In D103539#2794189 <https://reviews.llvm.org/D103539#2794189>, @jrtc27 wrote:

> Could you please re-upload the diff with full context?

Still applies.

In D103539#2794400 <https://reviews.llvm.org/D103539#2794400>, @jrtc27 wrote:

>> This undoes an ancient optimization in LLVM to support RISC-V's desire to over-emit relocations for link time relocation relaxation.
>
> Do you mean that we currently emit more relocations than we need to, or is this snark directed towards RISC-V's linker relaxations? If the latter, please remove such comments from your patch description.

Still applies.



================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:396
     return;
+
   MCContext &Ctx = Asm.getContext();
----------------
compnerd wrote:
> jrtc27 wrote:
> > Please don't "fix" unrelated whitespace
> Sure, from a final patch perhaps, but when working on something its very helpful.
Only locally. For reviewing it's a nuisance, so please remove them from the review.


================
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);
+
----------------
compnerd wrote:
> jrtc27 wrote:
> > I'm confused why this doesn't overwrite RI again with an R_RISCV_SUB8?
> Because there are two relocations which need to be emitted.  The problem here is that we need a second relocation on a different symbol.  The ideal thing would be that we generate two fixups in the first place.
I know what the code is trying to do, what I'm failing to grasp is why it actually works, it looks buggy to me, as in that it doesn't do what I think it does and what you say it's meant to, so I must be missing something given the tests still pass (I assume they do given you haven't touched any?).


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


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp:155
+    std::tuple<unsigned, unsigned> paired;
+
+    switch (Relocs[RI].Type) {
----------------
Use std::pair or a local struct type


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:61
+  MCA.setRelaxAll(STI.getFeatureBits()[RISCV::FeatureRelax]);
 }
 MCELFStreamer &RISCVTargetELFStreamer::getStreamer() {
----------------
Why has this suddenly appeared in version 2?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103539