[RISCV] Lower extern_weak symbols using the GOT for the medany model

[jrtc27]

Summary

Such symbols may be undefined at link time and thus resolve to 0, which
may be further than 2GiB away from PC, causing the immediate to be out
of range for PC-relative addressing. Using the GOT avoids this, and is
the approach taken by AArch64.

[jrtc27]

See: https://github.com/riscv/riscv-elf-psabi-doc/pull/201

[MaskRay]

Thanks! Two nits

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

2736

PseudoLGA

2759

IsExternWeak can be omitted

[MaskRay]

Add https://github.com/riscv/riscv-elf-psabi-doc/pull/201 to the description

[jrtc27]

Fixed copy paste error in comment

[jrtc27]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

2736

Already spotted and fixed :)

2759

What do you mean? If it's omitted then it defaults to false and we get the old codegen

[MaskRay]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

2759

Change the argument. The variable can be removed.

[jrtc27]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

2759

Well you could say the same about Ty and IsLocal, they're both used exactly once so could be inlined, but IMO this keeps it more readable; nested function calls can get rather messy.

[MaskRay]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

2759

IsExternWeak is a good candidate. Its expr isn't very long. IsLocal is long and leaving it as is is fine.

[reames]

It looks like this has been approved for roughly a year and yet hasn't landed. Could we get this rebased and landed?

I'm aware this is somewhat still an area of controversy in the psABI, but the GOT based lowering here is conservatively correct regardless of outcome there. If we do settle on the GOT required answer - which looks likely, having this in as early as possible should help minimize migration pain. If we decide on requiring the relaxation, this change does no harm and could be reverted later if desired. The only reason I can see to hold this if we think there's a potential performance concern, but if so, we really need to know that for the psABI discussion and exposing it by landing this and seeing what falls out seems like still a net win.

[MaskRay]

There is no controversy. The behavior on most targets for external weak symbol is to use GOT.

[jrtc27]

I was holding off until the LGA pseudo was accepted in riscv-asm-manual, which seems to finally have happened in March

[jrtc27]

Rebased, including to use riscv_lga ISD node instead and create a memop like for la

[arichardson]

LGTM if everyone else is happy with it.

Would it make sense to backport this patch series to the 15 release? I'd quite like to be able to make use of this.

[MaskRay]

Consider adding Link: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/201

This properly addresses the root cause leading to https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/197

[arichardson]

LLVM 16 has been released, can we get this into 17?

[asb]

I think all questions and review comments were resolved - anything you can see blocking this now @jrtc27?

[jrtc27]

In D107280#4260355, @asb wrote:

I think all questions and review comments were resolved - anything you can see blocking this now @jrtc27?

I think it just didn’t get landed because it needs rebasing to adapt to more recent changes in L(L)A handling. Will try and update in the coming days.

[jrtc27]

Rebased

[jrtc27]

Rebase on updated test

[MaskRay]

Looks great!

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

2736

Extern weak symbols, if unresolved, must be addressed indirectly ...

extern_weak is the IR representation related to the current module. Another TU may provide a definition. The original sentence is imprecise.

[MaskRay]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

2736

Hmm, better wording may be that "An unresolved undefined weak symbol has a value of zero, which may not be within +-2GiB of PC. Use a GOT-generating code sequence so that the zero address is representable." This may feel verbose. Feel free to simplify.

llvm/lib/Target/RISCV/RISCVISelLowering.h

void analyzeInputArgs(MachineFunction &MF, CCState &CCInfo,
                         const SmallVectorImpl<ISD::InputArg> &Ins, bool IsRet,
                         RISCVCCAssignFn Fn) const;
   void analyzeOutputArgs(MachineFunction &MF, CCState &CCInfo,
                          const SmallVectorImpl<ISD::OutputArg> &Outs,
                          bool IsRet, CallLoweringInfo *CLI,
                          RISCVCCAssignFn Fn) const;
 
   template <class NodeTy>
-  SDValue getAddr(NodeTy *N, SelectionDAG &DAG, bool IsLocal = true) const;
+  SDValue getAddr(NodeTy *N, SelectionDAG &DAG, bool IsLocal = true,
+                  bool IsExternWeak = false) const;
 
   SDValue getStaticTLSAddr(GlobalAddressSDNode *N, SelectionDAG &DAG,
                            bool UseGOT) const;
   SDValue getDynamicTLSAddr(GlobalAddressSDNode *N, SelectionDAG &DAG) const;
 
   SDValue lowerGlobalAddress(SDValue Op, SelectionDAG &DAG) const;
   SDValue lowerBlockAddress(SDValue Op, SelectionDAG &DAG) const;
   SDValue lowerConstantPool(SDValue Op, SelectionDAG &DAG) const;

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

 //===-- RISCVISelLowering.cpp - RISCV DAG Lowering Implementation  --------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file defines the interfaces that RISCV uses to lower LLVM code into a

 }
 
 static SDValue getTargetNode(JumpTableSDNode *N, SDLoc DL, EVT Ty,
                              SelectionDAG &DAG, unsigned Flags) {
   return DAG.getTargetJumpTable(N->getIndex(), Ty, Flags);
 }
 
 template <class NodeTy>
 SDValue RISCVTargetLowering::getAddr(NodeTy *N, SelectionDAG &DAG,
-                                     bool IsLocal) const {
+                                     bool IsLocal, bool IsExternWeak) const {
   SDLoc DL(N);
   EVT Ty = getPointerTy(DAG.getDataLayout());
 
   if (isPositionIndependent()) {
     SDValue Addr = getTargetNode(N, DL, Ty, DAG, 0);
     if (IsLocal)
       // Use PC-relative addressing to access the symbol. This generates the
       // pattern (PseudoLLA sym), which expands to (addi (auipc %pcrel_hi(sym))
case CodeModel::Small: {
     // Generate a sequence for accessing addresses within the first 2 GiB of
     // address space. This generates the pattern (addi (lui %hi(sym)) %lo(sym)).
     SDValue AddrHi = getTargetNode(N, DL, Ty, DAG, RISCVII::MO_HI);
     SDValue AddrLo = getTargetNode(N, DL, Ty, DAG, RISCVII::MO_LO);
     SDValue MNHi = SDValue(DAG.getMachineNode(RISCV::LUI, DL, Ty, AddrHi), 0);
     return SDValue(DAG.getMachineNode(RISCV::ADDI, DL, Ty, MNHi, AddrLo), 0);
   }
   case CodeModel::Medium: {
+    SDValue Addr = getTargetNode(N, DL, Ty, DAG, 0);
+    if (IsExternWeak)
+      // Extern weak symbols must be addressed indirectly as 0 may not be
+      // within 2GiB of PC. This generates the pattern (PseudoLGA sym), which
+      // expands to (ld (addi (auipc %got_pcrel_hi(sym)) %pcrel_lo(auipc))).
+      return SDValue(DAG.getMachineNode(RISCV::PseudoLGA, DL, Ty, Addr), 0);
+
     // Generate a sequence for accessing addresses within any 2GiB range within
     // the address space. This generates the pattern (PseudoLLA sym), which
     // expands to (addi (auipc %pcrel_hi(sym)) %pcrel_lo(auipc)).
-    SDValue Addr = getTargetNode(N, DL, Ty, DAG, 0);
     return SDValue(DAG.getMachineNode(RISCV::PseudoLLA, DL, Ty, Addr), 0);
   }
   }
 }
 
 SDValue RISCVTargetLowering::lowerGlobalAddress(SDValue Op,
                                                 SelectionDAG &DAG) const {
   SDLoc DL(Op);
   EVT Ty = Op.getValueType();
   GlobalAddressSDNode *N = cast<GlobalAddressSDNode>(Op);
   int64_t Offset = N->getOffset();
   MVT XLenVT = Subtarget.getXLenVT();
 
   const GlobalValue *GV = N->getGlobal();
   bool IsLocal = getTargetMachine().shouldAssumeDSOLocal(*GV->getParent(), GV);
-  SDValue Addr = getAddr(N, DAG, IsLocal);
+  bool IsExternWeak = GV->hasExternalWeakLinkage();
+  SDValue Addr = getAddr(N, DAG, IsLocal, IsExternWeak);
 
   // In order to maximise the opportunity for common subexpression elimination,
   // emit a separate ADD node for the global address offset instead of folding
   // it in the global address node. Later peephole optimisations may choose to
   // fold it back in when profitable.
   if (Offset != 0)
     return DAG.getNode(ISD::ADD, DL, Ty, Addr,
                        DAG.getConstant(Offset, DL, XLenVT));

llvm/test/CodeGen/RISCV/codemodel-lowering.ll

 ; RV32I-SMALL:       # %bb.0:
 ; RV32I-SMALL-NEXT:    lui a0, %hi(W)
 ; RV32I-SMALL-NEXT:    lw a0, %lo(W)(a0)
 ; RV32I-SMALL-NEXT:    ret
 ;
 ; RV32I-MEDIUM-LABEL: lower_extern_weak:
 ; RV32I-MEDIUM:       # %bb.0:
 ; RV32I-MEDIUM-NEXT:  .LBB4_1: # Label of block must be emitted
-; RV32I-MEDIUM-NEXT:    auipc a0, %pcrel_hi(W)
-; RV32I-MEDIUM-NEXT:    addi a0, a0, %pcrel_lo(.LBB4_1)
+; RV32I-MEDIUM-NEXT:    auipc a0, %got_pcrel_hi(W)
+; RV32I-MEDIUM-NEXT:    lw a0, %pcrel_lo(.LBB4_1)(a0)
 ; RV32I-MEDIUM-NEXT:    lw a0, 0(a0)
 ; RV32I-MEDIUM-NEXT:    ret
   %1 = load volatile i32, i32* @W
   ret i32 %1
 }