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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
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);
----------------
aeubanks wrote:
> 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
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157907