Page MenuHomePhabricator

[X86] Deduplicate symbol lowering logic, NFC

Authored by rnk on May 8 2019, 10:54 AM.



This refactors four pieces of code that create SDNodes for references to

  • normal global address lowering (LEA, MOV, etc)
  • callee global address lowering (CALL)
  • external symbol address lowering (LEA, MOV, etc)
  • external symbol address lowering (CALL)

Each of these pieces of code need to:

  • classify the reference
  • lower the symbol
  • emit a RIP wrapper if needed
  • emit a load if needed
  • add offsets if needed

I think handling them all in one place will make the code easier to
maintain in the future.

Diff Detail


Event Timeline

rnk created this revision.May 8 2019, 10:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2019, 10:54 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
rnk added a comment.May 15 2019, 4:20 PM

Ping, seems like a good simplification.

craig.topper added inline comments.May 15 2019, 4:38 PM
17249 ↗(On Diff #198697)

Why was this just checking OpFlag before instead of calling isGlobalRelativeToPICBase?

rnk marked an inline comment as done.May 16 2019, 11:38 AM
rnk added inline comments.
17249 ↗(On Diff #198697)

I think it's a bug. The vast majority of ExternalSymbols are for functions, so they don't reach this codepath, which is for LEA or memory operations. LowerExternalSymbol itself is really hard to reach, I only see these failures if I make it assert unconditionally:

Failing Tests (6):

LLVM :: CodeGen/X86/code-model-elf-memset.ll
LLVM :: CodeGen/X86/tls-windows-itanium.ll
LLVM :: CodeGen/X86/tls.ll
LLVM :: ExecutionEngine/MCJIT/test-fp.ll
LLVM :: ExecutionEngine/OrcMCJIT/test-fp.ll
LLVM :: ExecutionEngine/frem.ll

So, there are two cases where we come here present in the test suite:

  • large code model, which has a non-zero opflag and needs a PIC register,
  • tls, which comes with a zero opflag, and doesn't

There are a few opflags that don't involve a PIC register, like MO_GOTPCREL, MO_PLT, etc, but I can't come up with a test case to exercise them.

I like that you combined it for ease of use. We have OpenVMS-specific changes for our out-of-tree cross-compilers (based on 3.4.2) and it was even worse back then. Having it in one place makes it easier for us. We're currently rebasing for native compilers based on 8.0.0 but will probably rebase again quickly.

We have a unique memory model that is a mixture of small/medium/large. We allocate all static data in 32-bit space (and our stacks are 32-bits) but have code in 64-bit space so we always use GOT address even for "local" statics. We also always go through the GOT for calls to routines in different ELF sections since our linker might group them into clusters farther than 32-bits apart. Plus, our linker makes routine trampolines in 32-bit space for when somebody takes the "address" of a routine and expects to store it into a 32-bit location (our VAX heritage)

I'll be submitting a lightning talk this fall on "When 3 memory models on x86 isn't enough"

This revision is now accepted and ready to land.May 16 2019, 3:02 PM
This revision was automatically updated to reflect the committed changes.