[PATCH] D157907: [NFC] Refactor X86TargetLowering::getGlobalWrapperKind()

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:18246
+  Result =
+      DAG.getNode(getGlobalWrapperKind(nullptr, OpFlag), DL, PtrVT, Result);
   // With PIC, the address is actually $g + Offset.
----------------
rnk wrote:
> 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?
there was one last call site for `eh_sjlj_lsda` that I've fixed up


================
Comment at: llvm/lib/Target/X86/X86Subtarget.cpp:326
   // Determine the PICStyle based on the target selected.
-  if (!isPositionIndependent())
+  if (!isPositionIndependent() || TM.getCodeModel() == CodeModel::Large)
     setPICStyle(PICStyles::Style::None);
----------------
rnk wrote:
> 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?
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157907