[PATCH] D55335: [RISCV, WIP] Support assembling @plt symbol operands

[James Clarke via Phabricator via llvm-commits]

jrtc27 requested changes to this revision.
jrtc27 added a comment.
This revision now requires changes to proceed.

In D55335#1320688 <https://reviews.llvm.org/D55335#1320688>, @lewis-revill wrote:

> Ideally no. I guess it would be good to error when parsing rather than trying to emit a `R_RISCV_CALL_PLT` relocation for a non-call... I'm leaning towards checking `Operands[0]` at the moment but it depends what others think.


I personally don't particularly like having `parseBareSymbol` change behaviour based on the instruction name and would prefer to have a separate parser. You can then make `parseBareSymbol` reject anything with an `@` in it, and `parseCallTarget` (or whatever you want to name it) reject anything with an `@` in it after stripping off the optional `@plt`.

Other than the minor points given here, it seems good, hopefully we can get an amended version merged soon!



================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:115
   const MCExpr *CallExpr =
-      RISCVMCExpr::create(Expr, RISCVMCExpr::VK_RISCV_CALL, Ctx);
+      RISCVMCExpr::create(Expr, Kind, Ctx);
 
----------------
Isn't this just copying `Expr` if it's a `RISCVMCExpr` (assuming `Ctx` ends up being the same context)? If so, we can just re-use `Expr` for that case. But thinking about it further, can we not make sure that `PseudoCALL`/`PseudoTAIL` always get a `RISCVMCExpr`, either with a `VK_RISCV_CALL` or a `VK_RISCV_CALL_PLT`, so everywhere is always explicit about which it wants rather than having an implicit fallback?


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp:44
     OS << ')';
+  if (Kind == VK_RISCV_CALL_PLT)
+    OS << "@plt";
----------------
Personally I'd move this to before the closing `')'`. I know the two are mutually exclusive, but to me looking at the code it just feels slightly wrong that the `@plt` suffix isn't obviously attached directly to the symbol.


================
Comment at: test/MC/RISCV/tail-call.s:54
+# FIXUP: fixup A - offset: 0, value: foo at plt, kind:
\ No newline at end of file

----------------
Please include the trailing newline


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55335