[PATCH] D61907: [RISCV] Leave pcrel_hi/pcrel_lo fixup pairs unresolved

Fri Jul 12 02:28:50 PDT 2019

lewis-revill marked an inline comment as done.
lewis-revill added inline comments.

================
Comment at: test/MC/RISCV/linker-relaxation.s:125
 addi t1, t1, %pcrel_lo(2b)
-# NORELAX-RELOC-NOT: R_RISCV_PCREL_LO12_I
+# NORELAX-RELOC: R_RISCV_PCREL_LO12_I bar 0x4
 # NORELAX-RELOC-NOT: R_RISCV_RELAX
----------------
jrtc27 wrote:
> asb wrote:
> > asb wrote:
> > > Shouldn't be this be `.Ltmp1 0x0` just like in the RELAX-RELOC case?
> > Hi Lewis, did you have any thoughts on this?
> Yes, it should be. It's because `RISCVMCExpr::evaluateAsRelocatableImpl` evaluates it fully to the indirected symbol since `RAB.willForceRelocations` is false, unlike the RELAX-RELOC case.
So, if I understand correctly, `evaluatePCRelLo` will do it's work regardless of whether the relocation is actually evaluated, it just uses this 'best guess' function `RAB.willForceRelocations` and some of its own logic. Is this not a bug regardless of this patch? Are we relying on the logic in `shouldForceRelocations` and `evaluatePCRelLo` producing the exact same answer?

Are there uses of `evaluatePCRelLo` that aren't part of this assembler pipeline? Because if not then doesn't that imply that for this patch the entire `evaluatePCRelLo` functionality would be completely removed?

Repository:
  rL LLVM

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

https://reviews.llvm.org/D61907