[NFC] Refactor X86TargetLowering::getGlobalWrapperKind()

[aeubanks]

Summary

To simplify D150297.

We should be looking at OpFlags more.

[rnk]

llvm/lib/Target/X86/X86ISelLowering.cpp

18246

Are there more no-operand call sites? It seems that now they will do the wrong thing in the large code model. Should we remove the optional arguments to mandate that all sites pass OpFlag?

llvm/lib/Target/X86/X86Subtarget.cpp

326

This is subtle, can you expand on why we use the "None" PIC style for the Large PIC code model? Clearly, the ISel works out correctly regardless, but it's surprising. Would it make more sense to have a PICStyles::Style::LargePIC?

[aeubanks]

remove default args

llvm/lib/Target/X86/X86ISelLowering.cpp

18246

there was one last call site for eh_sjlj_lsda that I've fixed up

llvm/lib/Target/X86/X86Subtarget.cpp

326

basically it's so we don't go down the

if (Subtarget.isPICStyleRIPRel() &&
    (OpFlags == X86II::MO_NO_FLAG || OpFlags == X86II::MO_COFFSTUB ||
     OpFlags == X86II::MO_DLLIMPORT))
  return X86ISD::WrapperRIP;

code path above which isel doesn't know how to select under the large code model. I'm not sure that having a separate PICStyle is useful if we never actually use it

in general the X86II/X86ISD code seems to contain leaky abstractions

[rnk]

llvm/lib/Target/X86/X86Subtarget.cpp

326

Makes sense, but please add a comment on why the we use the None picstyle for the large+PIC config. Basically, the "none" style prevents us from forming any fancy combined memory operands, and forces all accesses to be indirect.

[aeubanks]

add comment

[rnk]

lgtm

[jasonmolenda]

Hi, this is causing a regression on the greendragon x86_64 lldb bot, it took a little while to repo and narrow down today. https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/ is failing on TestDequeFromStdModule.py, I can repo it on an intel mac. We're crashing under

LLVM ERROR: unable to evaluate offset to undefined symbol 'L21$pb'

4   liblldb.18.0.0git.dylib       	       0x1357b08a4 llvm::report_fatal_error(llvm::Twine const&, bool) + 388 (ErrorHandling.cpp:123)
5   liblldb.18.0.0git.dylib       	       0x13f73da2b getLabelOffset(llvm::MCAsmLayout const&, llvm::MCSymbol const&, bool, unsigned long long&) + 187 (MCFragment.cpp:107)
6   liblldb.18.0.0git.dylib       	       0x13f73aa48 getSymbolOffsetImpl(llvm::MCAsmLayout const&, llvm::MCSymbol const&, bool, unsigned long long&) + 72 (MCFragment.cpp:118)
7   liblldb.18.0.0git.dylib       	       0x13f73abde getSymbolOffsetImpl(llvm::MCAsmLayout const&, llvm::MCSymbol const&, bool, unsigned long long&) + 478 (MCFragment.cpp:143)
8   liblldb.18.0.0git.dylib       	       0x13f73ac56 llvm::MCAsmLayout::getSymbolOffset(llvm::MCSymbol const&) const + 38 (MCFragment.cpp:158)
9   liblldb.18.0.0git.dylib       	       0x13f6df97f llvm::MCAssembler::evaluateFixup(llvm::MCAsmLayout const&, llvm::MCFixup const&, llvm::MCFragment const*, llvm::MCValue&, unsigned long long&, bool&) const + 1151 (MCAssembler.cpp:262)
10  liblldb.18.0.0git.dylib       	       0x13f6e2aec llvm::MCAssembler::handleFixup(llvm::MCAsmLayout const&, llvm::MCFragment&, llvm::MCFixup const&) + 92 (MCAssembler.cpp:804)
11  liblldb.18.0.0git.dylib       	       0x13f6e362b llvm::MCAssembler::layout(llvm::MCAsmLayout&) + 2651 (MCAssembler.cpp:933)
12  liblldb.18.0.0git.dylib       	       0x13f6e40cd llvm::MCAssembler::Finish() + 77 (MCAssembler.cpp:944)
13  liblldb.18.0.0git.dylib       	       0x13f75b10b llvm::MCObjectStreamer::finishImpl() + 203 (MCObjectStreamer.cpp:932)
14  liblldb.18.0.0git.dylib       	       0x13f744709 (anonymous namespace)::MCMachOStreamer::finishImpl() + 761 (MCMachOStreamer.cpp:533)
15  liblldb.18.0.0git.dylib       	       0x13f785b10 llvm::MCStreamer::finish(llvm::SMLoc) + 224 (MCStreamer.cpp:1021)
16  liblldb.18.0.0git.dylib       	       0x1381ac986 llvm::AsmPrinter::doFinalization(llvm::Module&) + 6502 (AsmPrinter.cpp:2398)
17  liblldb.18.0.0git.dylib       	       0x13f1708ae llvm::FPPassManager::doFinalization(llvm::Module&) + 78 (LegacyPassManager.cpp:1499)
18  liblldb.18.0.0git.dylib       	       0x13f1699be (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) + 1358 (LegacyPassManager.cpp:1586)
19  liblldb.18.0.0git.dylib       	       0x13f1692ba llvm::legacy::PassManagerImpl::run(llvm::Module&) + 266 (LegacyPassManager.cpp:535)
20  liblldb.18.0.0git.dylib       	       0x13f170af1 llvm::legacy::PassManager::run(llvm::Module&) + 33 (LegacyPassManager.cpp:1677)
21  liblldb.18.0.0git.dylib       	       0x138d51d7d llvm::MCJIT::emitObject(llvm::Module*) + 477 (MCJIT.cpp:171)

@Michael137 might be better able to understand the nature of the regression, I'm not very familiar with this part of llvm, I'm trying to get repo steps for him on an aarch64 mac to build & run it under translation. I think we'll need to revert this until we can figure out the failure.

[Michael137]

In D157907#4605677, @jasonmolenda wrote:

Hi, this is causing a regression on the greendragon x86_64 lldb bot, it took a little while to repo and narrow down today. https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/ is failing on TestDequeFromStdModule.py, I can repo it on an intel mac. We're crashing under

LLVM ERROR: unable to evaluate offset to undefined symbol 'L21$pb'

4   liblldb.18.0.0git.dylib       	       0x1357b08a4 llvm::report_fatal_error(llvm::Twine const&, bool) + 388 (ErrorHandling.cpp:123)
5   liblldb.18.0.0git.dylib       	       0x13f73da2b getLabelOffset(llvm::MCAsmLayout const&, llvm::MCSymbol const&, bool, unsigned long long&) + 187 (MCFragment.cpp:107)
6   liblldb.18.0.0git.dylib       	       0x13f73aa48 getSymbolOffsetImpl(llvm::MCAsmLayout const&, llvm::MCSymbol const&, bool, unsigned long long&) + 72 (MCFragment.cpp:118)
7   liblldb.18.0.0git.dylib       	       0x13f73abde getSymbolOffsetImpl(llvm::MCAsmLayout const&, llvm::MCSymbol const&, bool, unsigned long long&) + 478 (MCFragment.cpp:143)
8   liblldb.18.0.0git.dylib       	       0x13f73ac56 llvm::MCAsmLayout::getSymbolOffset(llvm::MCSymbol const&) const + 38 (MCFragment.cpp:158)
9   liblldb.18.0.0git.dylib       	       0x13f6df97f llvm::MCAssembler::evaluateFixup(llvm::MCAsmLayout const&, llvm::MCFixup const&, llvm::MCFragment const*, llvm::MCValue&, unsigned long long&, bool&) const + 1151 (MCAssembler.cpp:262)
10  liblldb.18.0.0git.dylib       	       0x13f6e2aec llvm::MCAssembler::handleFixup(llvm::MCAsmLayout const&, llvm::MCFragment&, llvm::MCFixup const&) + 92 (MCAssembler.cpp:804)
11  liblldb.18.0.0git.dylib       	       0x13f6e362b llvm::MCAssembler::layout(llvm::MCAsmLayout&) + 2651 (MCAssembler.cpp:933)
12  liblldb.18.0.0git.dylib       	       0x13f6e40cd llvm::MCAssembler::Finish() + 77 (MCAssembler.cpp:944)
13  liblldb.18.0.0git.dylib       	       0x13f75b10b llvm::MCObjectStreamer::finishImpl() + 203 (MCObjectStreamer.cpp:932)
14  liblldb.18.0.0git.dylib       	       0x13f744709 (anonymous namespace)::MCMachOStreamer::finishImpl() + 761 (MCMachOStreamer.cpp:533)
15  liblldb.18.0.0git.dylib       	       0x13f785b10 llvm::MCStreamer::finish(llvm::SMLoc) + 224 (MCStreamer.cpp:1021)
16  liblldb.18.0.0git.dylib       	       0x1381ac986 llvm::AsmPrinter::doFinalization(llvm::Module&) + 6502 (AsmPrinter.cpp:2398)
17  liblldb.18.0.0git.dylib       	       0x13f1708ae llvm::FPPassManager::doFinalization(llvm::Module&) + 78 (LegacyPassManager.cpp:1499)
18  liblldb.18.0.0git.dylib       	       0x13f1699be (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) + 1358 (LegacyPassManager.cpp:1586)
19  liblldb.18.0.0git.dylib       	       0x13f1692ba llvm::legacy::PassManagerImpl::run(llvm::Module&) + 266 (LegacyPassManager.cpp:535)
20  liblldb.18.0.0git.dylib       	       0x13f170af1 llvm::legacy::PassManager::run(llvm::Module&) + 33 (LegacyPassManager.cpp:1677)
21  liblldb.18.0.0git.dylib       	       0x138d51d7d llvm::MCJIT::emitObject(llvm::Module*) + 477 (MCJIT.cpp:171)

@Michael137 might be better able to understand the nature of the regression, I'm not very familiar with this part of llvm, I'm trying to get repo steps for him on an aarch64 mac to build & run it under translation. I think we'll need to revert this until we can figure out the failure.

I can repro under Rosetta. Let me know if you need any details on how to repro or about the Module we're trying to compile.
We initialise our JIT ExecutionEngine as follows:

builder.setEngineKind(llvm::EngineKind::JIT)                              
    .setErrorStr(&error_string)                                           
    .setRelocationModel(triple.isOSBinFormatMachO() ? llvm::Reloc::PIC_   
                                                    : llvm::Reloc::Static)
    .setMCJITMemoryManager(std::make_unique<MemoryManager>(*this))        
    .setOptLevel(llvm::CodeGenOpt::Less);

And the getTargetMachine().getCodeModel() == Large (which is a codepath that changed in this patch).

[aeubanks]

I've reverted the patch, yeah seeing the IR would be useful

[Michael137]

In D157907#4607118, @aeubanks wrote:

I've reverted the patch, yeah seeing the IR would be useful

I won't be on my PC for the next 12 hours or so. I shared the IR with @jasonmolenda earlier today, hopefully he can forward it to you

If you have access to a x86_64 (or Rosetta) Darwin machine you should be able to repro it by compiling lldb/test/API/commands/expression/import-std-module/deque-basic/main.cpp with your changes and attaching lldb as follows:

./bin/lldb a.out -o "sett set target.import-std-module true" -o "br se -p return -X main" -o run -o "expr std::sort(a.begin(), a.end())"

[aeubanks]

In D157907#4607125, @Michael137 wrote:

In D157907#4607118, @aeubanks wrote:

I've reverted the patch, yeah seeing the IR would be useful

I won't be on my PC for the next 12 hours or so. I shared the IR with @jasonmolenda earlier today, hopefully he can forward it to you

If you have access to a x86_64 (or Rosetta) Darwin machine you should be able to repro it by compiling lldb/test/API/commands/expression/import-std-module/deque-basic/main.cpp with your changes and attaching lldb as follows:

./bin/lldb a.out -o "sett set target.import-std-module true" -o "br se -p return -X main" -o run -o "expr std::sort(a.begin(), a.end())"

I'm not in a rush to get this relanded so posting the IR later is fine

[Michael137]

Hey,

I attached the module it's blowing up on

• module.ll319 KBDownload

[Michael137]

In D157907#4607147, @aeubanks wrote:

In D157907#4607125, @Michael137 wrote:

In D157907#4607118, @aeubanks wrote:

I've reverted the patch, yeah seeing the IR would be useful

I won't be on my PC for the next 12 hours or so. I shared the IR with @jasonmolenda earlier today, hopefully he can forward it to you

If you have access to a x86_64 (or Rosetta) Darwin machine you should be able to repro it by compiling lldb/test/API/commands/expression/import-std-module/deque-basic/main.cpp with your changes and attaching lldb as follows:

./bin/lldb a.out -o "sett set target.import-std-module true" -o "br se -p return -X main" -o run -o "expr std::sort(a.begin(), a.end())"

I'm not in a rush to get this relanded so posting the IR later is fine

I sent you the module file. You should be able to repro the crash without going through LLDB by running:

./bin/lli --relocation-model=pic --code-model=large --jit-kind=mcjit --entry-function="_Z12\$__lldb_exprPv" module.ll

[aeubanks]

reduced

target triple = "x86_64-apple-macosx13.0.0"

define void @f() noinline optnone {
bb:
  switch i64 0, label %bb1 [
    i64 0, label %bb1
    i64 2, label %bb2
    i64 5, label %bb4
    i64 4, label %bb3
  ]

bb1:                                              ; preds = %bb, %bb
  ret void

bb2:                                              ; preds = %bb
  ret void

bb3:                                              ; preds = %bb
  ret void

bb4:                                              ; preds = %bb
  ret void
}

$ lli --relocation-model=pic --code-model=large --jit-kind=mcjit --entry-function=f /tmp/a.ll

has something to do with jump tables (which are actually known to be an issue in clang's large code model)

https://github.com/llvm/llvm-project/blob/5c92c9f34a7dba804479acef62c576d1a170ef1f/llvm/lib/Target/X86/X86ISelLoweringCall.cpp#L515

[rnk]

Regarding large code model jump tables, I think I filed an issue for alternative, relocation-less jump table encodings: https://github.com/llvm/llvm-project/issues/62894

[aeubanks]

https://reviews.llvm.org/D159297 fixes the jump table issue

llvm/lib/Target/X86/X86ISelLowering.cpp

 
 // Returns the appropriate wrapper opcode for a global reference.
 unsigned X86TargetLowering::getGlobalWrapperKind(
     const GlobalValue *GV, const unsigned char OpFlags) const {
   // References to absolute symbols are never PC-relative.
   if (GV && GV->isAbsoluteSymbolRef())
     return X86ISD::Wrapper;
 
-  CodeModel::Model M = getTargetMachine().getCodeModel();
+  // The following OpFlags under RIP-rel PIC use RIP.
   if (Subtarget.isPICStyleRIPRel() &&
-      (M == CodeModel::Small || M == CodeModel::Kernel))
+      (OpFlags == X86II::MO_NO_FLAG || OpFlags == X86II::MO_COFFSTUB ||
+       OpFlags == X86II::MO_DLLIMPORT))
     return X86ISD::WrapperRIP;
 
   // In the medium model, functions can always be referenced RIP-relatively,
   // since they must be within 2GiB. This is also possible in non-PIC mode, and
   // shorter than the 64-bit absolute immediate that would otherwise be emitted.
-  if (M == CodeModel::Medium && isa_and_nonnull<Function>(GV))
+  if (getTargetMachine().getCodeModel() == CodeModel::Medium &&
+      isa_and_nonnull<Function>(GV))
     return X86ISD::WrapperRIP;
 
   // GOTPCREL references must always use RIP.
   if (OpFlags == X86II::MO_GOTPCREL || OpFlags == X86II::MO_GOTPCREL_NORELAX)
     return X86ISD::WrapperRIP;
 
   return X86ISD::Wrapper;
 }
X86TargetLowering::LowerConstantPool(SDValue Op, SelectionDAG &DAG) const {
   // In PIC mode (unless we're in RIPRel PIC mode) we add an offset to the
   // global base reg.
   unsigned char OpFlag = Subtarget.classifyLocalReference(nullptr);
 
   auto PtrVT = getPointerTy(DAG.getDataLayout());
   SDValue Result = DAG.getTargetConstantPool(
       CP->getConstVal(), PtrVT, CP->getAlign(), CP->getOffset(), OpFlag);
   SDLoc DL(CP);
-  Result = DAG.getNode(getGlobalWrapperKind(), DL, PtrVT, Result);
+  Result =
+      DAG.getNode(getGlobalWrapperKind(nullptr, OpFlag), DL, PtrVT, Result);
   // With PIC, the address is actually $g + Offset.
   if (OpFlag) {
     Result =
         DAG.getNode(ISD::ADD, DL, PtrVT,
                     DAG.getNode(X86ISD::GlobalBaseReg, SDLoc(), PtrVT), Result);
   }
 
   return Result;
 }
 
 SDValue X86TargetLowering::LowerJumpTable(SDValue Op, SelectionDAG &DAG) const {
   JumpTableSDNode *JT = cast<JumpTableSDNode>(Op);
 
   // In PIC mode (unless we're in RIPRel PIC mode) we add an offset to the
   // global base reg.
   unsigned char OpFlag = Subtarget.classifyLocalReference(nullptr);
 
   auto PtrVT = getPointerTy(DAG.getDataLayout());
   SDValue Result = DAG.getTargetJumpTable(JT->getIndex(), PtrVT, OpFlag);
   SDLoc DL(JT);
-  Result = DAG.getNode(getGlobalWrapperKind(), DL, PtrVT, Result);
+  Result =
+      DAG.getNode(getGlobalWrapperKind(nullptr, OpFlag), DL, PtrVT, Result);
 
   // With PIC, the address is actually $g + Offset.
   if (OpFlag)
     Result =
         DAG.getNode(ISD::ADD, DL, PtrVT,
                     DAG.getNode(X86ISD::GlobalBaseReg, SDLoc(), PtrVT), Result);
 
   return Result;
X86TargetLowering::LowerBlockAddress(SDValue Op, SelectionDAG &DAG) const {
   // Create the TargetBlockAddressAddress node.
   unsigned char OpFlags =
     Subtarget.classifyBlockAddressReference();
   const BlockAddress *BA = cast<BlockAddressSDNode>(Op)->getBlockAddress();
   int64_t Offset = cast<BlockAddressSDNode>(Op)->getOffset();
   SDLoc dl(Op);
   auto PtrVT = getPointerTy(DAG.getDataLayout());
   SDValue Result = DAG.getTargetBlockAddress(BA, PtrVT, Offset, OpFlags);
-  Result = DAG.getNode(getGlobalWrapperKind(), dl, PtrVT, Result);
+  Result =
+      DAG.getNode(getGlobalWrapperKind(nullptr, OpFlags), dl, PtrVT, Result);
 
   // With PIC, the address is actually $g + Offset.
   if (isGlobalRelativeToPICBase(OpFlags)) {
     Result = DAG.getNode(ISD::ADD, dl, PtrVT,
                          DAG.getNode(X86ISD::GlobalBaseReg, dl, PtrVT), Result);
   }
 
   return Result;

llvm/lib/Target/X86/X86Subtarget.cpp

X86Subtarget::X86Subtarget(const Triple &TT, StringRef CPU, StringRef TuneCPU,
     : X86GenSubtargetInfo(TT, CPU, TuneCPU, FS),
       PICStyle(PICStyles::Style::None), TM(TM), TargetTriple(TT),
       StackAlignOverride(StackAlignOverride),
       PreferVectorWidthOverride(PreferVectorWidthOverride),
       RequiredVectorWidth(RequiredVectorWidth),
       InstrInfo(initializeSubtargetDependencies(CPU, TuneCPU, FS)),
       TLInfo(TM, *this), FrameLowering(*this, getStackAlignment()) {
   // Determine the PICStyle based on the target selected.
-  if (!isPositionIndependent())
+  if (!isPositionIndependent() || TM.getCodeModel() == CodeModel::Large)
     setPICStyle(PICStyles::Style::None);
   else if (is64Bit())
     setPICStyle(PICStyles::Style::RIPRel);
   else if (isTargetCOFF())
     setPICStyle(PICStyles::Style::None);
   else if (isTargetDarwin())
     setPICStyle(PICStyles::Style::StubPIC);
   else if (isTargetELF())