[PATCH] D66340: [RISCV] Support NonLazyBind

[James Clarke via Phabricator via llvm-commits]

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

Looks sensible in general, just various minor points.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2192
   // TargetGlobalAddress/TargetExternalSymbol node so that legalize won't
   // split it and then direct call can be matched by PseudoCALL.
   if (GlobalAddressSDNode *S = dyn_cast<GlobalAddressSDNode>(Callee)) {
----------------
This comment probably needs updating to reflect the fact that GlobalAddress/ExternalSymbol may now become a normal node and indirectly called if using NonLazyBind.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2197
+    unsigned OpFlags =
+             Subtarget.classifyGlobalFunctionReference(GV, getTargetMachine());
+
----------------
This should only be an additional 4 spaces of indentation.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2203
+      Callee = DAG.getTargetGlobalAddress(GV, DL, PtrVT, 0, OpFlags);
 
   } else if (ExternalSymbolSDNode *S = dyn_cast<ExternalSymbolSDNode>(Callee)) {
----------------
Please remove this extra line.


================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.cpp:54
+    const GlobalValue *GV, const TargetMachine &TM) const {
+  auto *F = dyn_cast<Function>(GV);
+  if (F && !TM.shouldAssumeDSOLocal(*GV->getParent(), GV)) {
----------------
The `dyn_cast` seems a bit weird but AArch64 and X86 (the only other backends supporting this) both do that so I guess it's fine. However, the `MO_CALL` fallback case is a change of behaviour; non-function non-DSO-locals would previously have still been called with an `MO_PLT`, so the `F &&` should be part of the `F->hasFnAttribute` condition.


================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.h:66
+  /// Return how the function given by GV should be invoked:
+  /// via PLT, by taking its address from GOT, or with a direct call.
+  unsigned char classifyGlobalFunctionReference(const GlobalValue *GV,
----------------
"the PLT" and "the GOT".


================
Comment at: llvm/test/CodeGen/RISCV/no-plt.ll:1
+; RUN: llc -mtriple=riscv64-linux-gnu --relocation-model=pic < %s | FileCheck %s
+
----------------
Please do this for `-mtriple=riscv32` and `-mtriple=riscv64` (no `-linux-gnu` is generally used in the CodeGen tests unless it's necessary), and use a single `-` for `-relocation-model`.


================
Comment at: llvm/test/CodeGen/RISCV/no-plt.ll:6
+define void @no_PLT_call() nounwind {
+  ;CHECK-LABEL: no_PLT_call:
+
----------------
Please use `update_llc_test_checks.py` (and my own personal opinion is this would look nicer with a lowercase `plt` in the function name).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66340