[PATCH] D127549: RISCV: handle 64-bit PCREL data relocations

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added inline comments.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:232-234
+    // the fixups here as we should relax the pair-wise relocation to a
+    // R_RISCV_[32|64|128]_PCREL instead.
+
----------------
luismarques wrote:
> This point is a bit of a nit-pick but that optimization isn't limited to when we are generating code for the compact code model. Rather, to define the compact code mode it's necessary to define that new relocation. Once we have support for that relocation (in the object code generator and the linker) we should be able to use it, whatever code model we are targeting. (The same point applies to the patch summary).
I agree, it isn't limited to the compact code-model; were the relocation available in general, that would be preferable.  I can try to reword this to make that more explicit.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:236-242
+    if (A.isInSection())
+      return !IsDebugOrEHFrameSection(A.getSection());
+    if (B.isInSection())
+      return !IsDebugOrEHFrameSection(B.getSection());
+    // as well as for absolute symbols.
+    return !A.getName().empty() || !B.getName().empty();
   }
----------------
luismarques wrote:
> luismarques wrote:
> > compnerd wrote:
> > > craig.topper wrote:
> > > > compnerd wrote:
> > > > > craig.topper wrote:
> > > > > > Should relaxation here be relocation?
> > > > > In the case of a compact code model, we can relax the pair of relocations to a single relocation.  You would still have a relocation, it just is a nominally smaller file, and fewer relocations for the linker to process.
> > > > I was asking about “Avoid relaxation for symbolic differences…”. Is relaxation the right word there?
> > > Right; I believe that is the term here - we are technically relaxing the sequence by replacing the two relocations with one.
> > I've never seen "relaxation" being used that way. While that seems a reasonable generalization of how the term is typically used in compiler land, I suspect most people are going to be confused by the comment.
> Are we testing all of these conditions?
This isn't new - this is the existing code path.  I would assume that they are are being tested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127549