[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

5310

PseudoLGA

5337

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

5310

Already spotted and fixed :)

5337

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

5337

Change the argument. The variable can be removed.

[jrtc27]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

5337

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

5337

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

5310

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

5310

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;
   SDValue lowerJumpTable(SDValue Op, SelectionDAG &DAG) const;

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

 
 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());
 
   // When HWASAN is used and tagging of global variables is enabled
   // they should be accessed via the GOT, since the tagged address of a global
   // is incompatible with existing code models. This also applies to non-pic
   // mode.
   if (isPositionIndependent() || Subtarget.allowTaggedGlobals()) {
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 = DAG.getNode(RISCVISD::HI, DL, Ty, AddrHi);
     return DAG.getNode(RISCVISD::ADD_LO, DL, Ty, MNHi, AddrLo);
   }
   case CodeModel::Medium: {
+    SDValue Addr = getTargetNode(N, DL, Ty, DAG, 0);
+    if (IsExternWeak) {
+      // An extern weak symbol may be undefined, i.e. have value 0, which may
+      // not be within 2GiB of PC, so use GOT-indirect addressing to access the
+      // symbol. This generates the pattern (PseudoLGA sym), which expands to
+      // (ld (addi (auipc %got_pcrel_hi(sym)) %pcrel_lo(auipc))).
+      MachineFunction &MF = DAG.getMachineFunction();
+      MachineMemOperand *MemOp = MF.getMachineMemOperand(
+          MachinePointerInfo::getGOT(MF),
+          MachineMemOperand::MOLoad | MachineMemOperand::MODereferenceable |
+              MachineMemOperand::MOInvariant,
+          LLT(Ty.getSimpleVT()), Align(Ty.getFixedSizeInBits() / 8));
+      SDValue Load =
+          DAG.getMemIntrinsicNode(RISCVISD::LGA, DL,
+                                  DAG.getVTList(Ty, MVT::Other),
+                                  {DAG.getEntryNode(), Addr}, Ty, MemOp);
+      return Load;
+    }
+
     // 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 DAG.getNode(RISCVISD::LLA, DL, Ty, Addr);
   }
   }
 }
 
 SDValue RISCVTargetLowering::lowerGlobalAddress(SDValue Op,
                                                 SelectionDAG &DAG) const {
   GlobalAddressSDNode *N = cast<GlobalAddressSDNode>(Op);
   assert(N->getOffset() == 0 && "unexpected offset in global node");
-  return getAddr(N, DAG, N->getGlobal()->isDSOLocal());
+  const GlobalValue *GV = N->getGlobal();
+  return getAddr(N, DAG, GV->isDSOLocal(), GV->hasExternalWeakLinkage());
 }
 
 SDValue RISCVTargetLowering::lowerBlockAddress(SDValue Op,
                                                SelectionDAG &DAG) const {
   BlockAddressSDNode *N = cast<BlockAddressSDNode>(Op);
 
   return getAddr(N, DAG);
 }

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:  .Lpcrel_hi3:
-; RV32I-MEDIUM-NEXT:    auipc a0, %pcrel_hi(W)
+; RV32I-MEDIUM-NEXT:    auipc a0, %got_pcrel_hi(W)
 ; RV32I-MEDIUM-NEXT:    lw a0, %pcrel_lo(.Lpcrel_hi3)(a0)
+; RV32I-MEDIUM-NEXT:    lw a0, 0(a0)
 ; RV32I-MEDIUM-NEXT:    ret
   %1 = load volatile i32, ptr @W
   ret i32 %1
 }