[PATCH] D123211: [flang][driver] Add support for generating LLVM bytecode files

[Diana Picus via Phabricator via cfe-commits]

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

In D123211#3433059 <https://reviews.llvm.org/D123211#3433059>, @awarzynski wrote:

> Thanks for taking a look Diana!
>
>> Why not use the BackendAction for this?
>
> This action only requires the middle-end in LLVM :) The `BackendAction` will use the backend, but that's not needed here.

Is there actually a significant difference, besides the naming (which is easy to change)? AFAICT the BackendAction isn't initializing anything backend-related except very close to where it's using it, so that can happen inside a switch/if branch.

>> There seems to be a lot of shared code, up until the point where you create and use the pass manager
>
> This is the bit that's shared:
>
>   CompilerInstance &ci = this->instance();
>   // Generate an LLVM module if it's not already present (it will already be
>   // present if the input file is an LLVM IR/BC file).
>   if (!llvmModule)
>     GenerateLLVMIR();
>   
>   // Create and configure `Target`
>   std::string error;
>   std::string theTriple = llvmModule->getTargetTriple();
>   const llvm::Target *theTarget =
>       llvm::TargetRegistry::lookupTarget(theTriple, error);
>   assert(theTarget && "Failed to create Target");
>   
>   // Create and configure `TargetMachine`
>   std::unique_ptr<llvm::TargetMachine> TM;
>   
>   TM.reset(theTarget->createTargetMachine(theTriple, /*CPU=*/"",
>       /*Features=*/"", llvm::TargetOptions(), llvm::None));
>   assert(TM && "Failed to create TargetMachine");
>   llvmModule->setDataLayout(TM->createDataLayout());
>
> I wouldn't say it's "a lot", but probably enough for a dedicated method. I've been conservative with regard to sharing code as I anticipate things to change in the future (I expect `-O{0|1|2|3|s|z}` to complicate the logic a bit).

I was thinking also about the code for generating the output file, which can be folded into the switch from BackendAction. If you consider that too, it becomes a very large percentage of EmitLLVMBitcodeAction::ExecuteAction that can be shared.

>> (and in the future, when the backend switches to the new pass manager, there will be even more shared code).
>
> I'm not sure about the timelines for this. And even then, the logic might be quite different (again, middle-end vs back-end).

Ok. IMO the template method pattern would work well here (or less formally, just a simple switch to the same effect), but I can understand if you think it's premature to go that route. 
No other objections from my side, thanks :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123211