[PATCH] D61690: [X86] Deduplicate symbol lowering logic, NFC

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked an inline comment as done.
rnk added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:17249
-  // With PIC, the address is actually $g + Offset.
-  if (OpFlag) {
-    Result =
----------------
craig.topper wrote:
> Why was this just checking OpFlag before instead of calling isGlobalRelativeToPICBase?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61690/new/

https://reviews.llvm.org/D61690