[PATCH] D47211: [X86] Implement more of x86-64 large and medium PIC code models

[Eric Christopher via Phabricator via llvm-commits]

echristo added a comment.

Sorry about the late review. I've added a lot of comment and documentation requests for the code and a couple of questions.



================
Comment at: llvm/trunk/lib/Target/X86/X86ISelDAGToDAG.cpp:946
   CodeModel::Model M = TM.getCodeModel();
-  if (Subtarget->is64Bit() && M != CodeModel::Small && M != CodeModel::Kernel)
+  if (Subtarget->is64Bit() && M != CodeModel::Small && M != CodeModel::Kernel &&
+      !(M == CodeModel::Medium && IsRIPRel))
----------------
That's a lot of negatives :)

Also doesn't entirely look like the comment describes what's going on here - there's no comment about medium and riprel here. Otherwise you should be able to just use "M == CodeModel:Large" or something?

Thoughts?


================
Comment at: llvm/trunk/lib/Target/X86/X86InstrInfo.cpp:12726
       // uses RIP relative addressing.
-      if (STI.is64Bit())
+      if (STI.is64Bit() && (TM->getCodeModel() == CodeModel::Small ||
+                            TM->getCodeModel() == CodeModel::Kernel))
----------------
Comment update?


================
Comment at: llvm/trunk/lib/Target/X86/X86Subtarget.cpp:117
   // Large model never uses stubs.
-  if (TM.getCodeModel() == CodeModel::Large)
+  if (TM.getCodeModel() == CodeModel::Large && !isPositionIndependent())
     return X86II::MO_NO_FLAG;
----------------
Comment update.


================
Comment at: llvm/trunk/lib/Target/X86/X86Subtarget.cpp:139
 
-  if (is64Bit())
+  if (is64Bit() && TM.getCodeModel() != CodeModel::Large)
     return X86II::MO_GOTPCREL;
----------------
This could probably use a comment as well.


================
Comment at: llvm/trunk/lib/Target/X86/X86TargetMachine.cpp:159
 static Reloc::Model getEffectiveRelocModel(const Triple &TT,
+                                           bool JIT,
                                            Optional<Reloc::Model> RM) {
----------------
Not sure I understand this change?


Repository:
  rL LLVM

https://reviews.llvm.org/D47211