This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Refactor X86TargetLowering::getGlobalWrapperKind()
ClosedPublic

Authored by aeubanks on Aug 14 2023, 11:28 AM.

Diff Detail

Event Timeline

aeubanks created this revision.Aug 14 2023, 11:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2023, 11:28 AM
aeubanks requested review of this revision.Aug 14 2023, 11:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2023, 11:28 AM
rnk added inline comments.Aug 14 2023, 12:09 PM
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 updated this revision to Diff 550084.Aug 14 2023, 2:06 PM

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 added inline comments.Aug 16 2023, 11:43 AM
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 updated this revision to Diff 551639.Aug 18 2023, 2:25 PM

add comment

rnk accepted this revision.Aug 18 2023, 2:30 PM

lgtm

This revision is now accepted and ready to land.Aug 18 2023, 2:30 PM
This revision was automatically updated to reflect the committed changes.

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 added a comment.EditedAug 22 2023, 7:01 AM

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).

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

aeubanks reopened this revision.Aug 22 2023, 9:06 AM
This revision is now accepted and ready to land.Aug 22 2023, 9:06 AM

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'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

Hey,

I attached the module it's blowing up on

Michael137 added a comment.EditedAug 23 2023, 11:43 PM

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

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 added a comment.Aug 28 2023, 1:48 PM

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