[PATCH] D103539: RISCV: adjust handling of relocation emission for RISCV

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd marked 13 inline comments as done.
compnerd added inline comments.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:195
+
+  static bool useLinkerRelaxation(MCContext &C, const MCExpr *Value,
+                                  const MCExpr *&LHS, const MCExpr *&RHS) {
----------------
nickdesaulniers wrote:
> does this need to be `static`?
No, it doesn't need to be `static`.  It is more at a conceptual level not something tied into the state of the streamer - it simply looks at an expression and tells you if you need a paired relocation for handling this.  BTW, I welcome bike-shedding on this function's name - I dislike the current name of this function.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:208
+    const auto &A = E.getSymA()->getSymbol();
+    const auto &B = E.getSymA()->getSymbol();
+
----------------
nickdesaulniers wrote:
> Should this call `getSymB()` rather than `getSymA()`?
Move along, nothing to see here.  That is rather embarrassing.  Thank you for flagging this.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:230-233
+    if (!useLinkerRelaxation(getContext(), Value, A, B))
+      return MCELFStreamer::emitValueImpl(Value, Size, Loc);
+
+    MCStreamer::emitValueImpl(Value, Size, Loc);
----------------
nickdesaulniers wrote:
> Should this be:
> ```
> MCStreamer::emitValueImpl(Value, Size, Loc);
> if (!useLinkerRelaxation(getContext(), Value, A, B))
>   return;
> ```
No, they are going down different functions - MCELFStreamer vs MCStreamer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103539