[PATCH] D149288: [X86] Introduce a large data threshold for the medium code model

[Reid Kleckner via Phabricator via llvm-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm



================
Comment at: llvm/tools/llc/llc.cpp:498
   auto InitializeOptions = [&](const Triple &TheTriple) {
     Options = codegen::InitTargetOptionsFromCodeGenFlags(TheTriple);
 
----------------
aeubanks wrote:
> rnk wrote:
> > Should the threshold be a `TargetOption`? Would that avoid the need for the new TargetMachine field?
> it should go along with the code model since they're tied together. I do think putting all these options (code model, relocation model) in `TargetOptions` makes sense, it fits with lots of existing options there. if we put all these options into `TargetOptions` it'd simplify the `TargetMachine` constructors. if people think this makes sense I can do that cleanup first
I think what I'd like to do is to minimize the number of changes to the `createTargetMachine` prototype, since it's called in many places. It has many optional parameters, and I suspect that most callers accept the defaults:

  TargetMachine *createTargetMachine(
      StringRef TT, StringRef CPU, StringRef Features,
      const TargetOptions &Options,
      std::optional<Reloc::Model> RM, // No default, but is std::optional
      std::optional<CodeModel::Model> CM = std::nullopt, // Optional, with a default.
      CodeGenOpt::Level OL = CodeGenOpt::Default, // has a default, this will change to an enum class
      bool JIT = false // Boolean optional parameters are unreadable.
  ) const {

Maybe the first change is to move the relocation and code model into target options. We probably have to leave the optimization level alone, since there are multiple callers of `TargetMachine::setOptLevel`, and it's not really an immutable option, it varies from function to function.

That said, if you want to land this now, and defer that work, I'm OK with that, this change doesn't affect the `createTargetMachine` prototype, it's the CodeGenLevel one that does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149288