[PATCH] D73211: [RISCV] Fix evaluating %pcrel_lo against global and weak symbols

[James Clarke via Phabricator via llvm-commits]

jrtc27 marked 5 inline comments as done.
jrtc27 added a comment.

In D73211#1834739 <https://reviews.llvm.org/D73211#1834739>, @efriedma wrote:

> This approach seems much easier to understand.
>
> Can you add a testcase for the case where the auipc and addi are in different fragments, like I mentioned in D71978 <https://reviews.llvm.org/D71978>?


Thanks for the quick review. I shall add that and address the comments.



================
Comment at: llvm/include/llvm/MC/MCAsmBackend.h:114
+                                   const MCFixup &Fixup, const MCFragment *DF,
+                                   MCValue &Target, uint64_t &Value,
+                                   bool &WasForced) {
----------------
efriedma wrote:
> Can the "Target" be a const reference?
Yes, it probably should be. In theory a backend could leave the fixup unresolved but mutate the target, however that seems rare (and is precisely what we're trying to avoid with RISC-V...). If a backend grows to need it it can always be de-consted later.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:279
+    const MCExpr *AUIPCExpr = AUIPCFixup->getValue();
+    if (!AUIPCExpr->evaluateAsRelocatable(AUIPCTarget, &Layout, AUIPCFixup))
+      return true;
----------------
efriedma wrote:
> MCAssembler::evaluateFixup emits an error if evaluateAsRelocatable fails... maybe worth adding a comment if you're intentionally not emitting a similar error?
Yes, it's deliberately omitted since evaluateFixup will emit the same error when it sees the real pcrel_hi. I will add a comment.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:283
+    if (const MCSymbolRefExpr *RefB = Target.getSymB()) {
+      if (RefB->getKind() != MCSymbolRefExpr::VK_None)
+        return true;
----------------
efriedma wrote:
> If "Target.getSymB()" is true, should getPCRelHiFixup fail?
That was intended to be AUIPCTarget, but that is already handled below in general regardless of the variant kind so I will just remove this.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:323
+
+  if (shouldForceRelocation(Asm, Fixup, Target)) {
+    WasForced = true;
----------------
efriedma wrote:
> `shouldForceRelocation(Asm, AUIPCFixup, AUIPCTarget)`?  (I think the difference is significant; please make sure we have a testcase that covers it.)
It currently doesn't matter, the intent was only to pick up the FeatureRelax etc code path. But you're right, if I pass AUIPCFixup, things are better, and I can delete the WasForced case statements above (validating it's a valid fixup should already have happened in getPCRelHiFixup).


================
Comment at: llvm/test/MC/RISCV/pcrel-fixups.s:86
+
+	.weak	global_function
+	.type	global_function, at function
----------------
efriedma wrote:
> Did you mean to make both global_function and weak_function weak?
We both know the answer is no :) will fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73211