Page MenuHomePhabricator

[flang][driver] Add support for generating LLVM bytecode files
ClosedPublic

Authored by awarzynski on Apr 6 2022, 5:08 AM.

Details

Summary

Support for generating LLVM BC files is added in Flang's compiler and
frontend drivers. This requires the BitcodeWriterPass pass to be run
on the input LLVM IR module and is implemented as a dedicated frontend
aciton. The new functionality as seen by the user (compiler driver):

flang-new -c -emit-llvm file.90

or (frontend driver):

flang-new -fc1 -emit-llvm-bc file.f90

The new behaviour is consistent with clang and clang -cc1.

Diff Detail

Event Timeline

awarzynski created this revision.Apr 6 2022, 5:08 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 6 2022, 5:08 AM
Herald added a subscriber: mgorny. · View Herald Transcript
awarzynski requested review of this revision.Apr 6 2022, 5:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2022, 5:08 AM
rovka added a comment.Apr 6 2022, 5:46 AM

Why not use the BackendAction for this? There seems to be a lot of shared code, up until the point where you create and use the pass manager (and in the future, when the backend switches to the new pass manager, there will be even more shared code).

flang/lib/Frontend/FrontendActions.cpp
483

Nit: I think you can move these closer to their first use.

flang/test/CMakeLists.txt
58

Were you going to add a test that uses this?

awarzynski updated this revision to Diff 420821.Apr 6 2022, 6:17 AM

Add missing test

awarzynski updated this revision to Diff 420862.Apr 6 2022, 7:42 AM

Move code a bit, add comments

flang/lib/Frontend/FrontendActions.cpp
483

Thanks, I thought that I _did_ move it :)

flang/test/CMakeLists.txt
58

Sorry, forgot to add in the original revision. Should be there now.

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.

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).

(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).

rovka accepted this revision.Apr 7 2022, 1:33 AM

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 :)

This revision is now accepted and ready to land.Apr 7 2022, 1:33 AM
unterumarmung accepted this revision.Apr 7 2022, 6:53 AM
ekieri accepted this revision.Apr 11 2022, 2:05 PM

One nit/question inline, otherwise LGTM.

flang/lib/Frontend/FrontendActions.cpp
491

Is there a reason why use TM.reset instead of initialising TM like you do with os below?

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.

Currently the driver does the bare minimum to generate machine code, hence in practice both actions are near identical. I expect this to change, but I might be wrong :)

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.

Ack!

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.

Definitely not objecting code re-use! :) But I would like to wait for a few patches for optimisation pipelines to land first. And if anyone feels differently and sends a patch, I'd be alright with that.

flang/lib/Frontend/FrontendActions.cpp
491

Good question!

Note that createTargetMachine that I use below returns a plain pointer. createDefaultOutputFile that I use for initialising os below returns std::unique_ptr<raw_pwrite_stream>. IIUC, I can only reset a unique_ptr (like TM) with a plain pointer.

Have I missed anything?

ekieri added inline comments.Apr 12 2022, 1:35 PM
flang/lib/Frontend/FrontendActions.cpp
491

Well, I had missed that. Thanks! Let's see if I got it right this time.

So unique_ptr has an _explicit_ constructor taking a raw pointer (constructor (2)). So as you say (if I interpreted you correctly),

std::unique_ptr<llvm::TargetMachine> TM = theTarget->createTargetMachine(...);

does not work, as it requires an implicit conversion from raw to unique pointer. But constructor syntax should (and does for me) work:

std::unique_ptr<llvm::TargetMachine> TM(theTarget->createTargetMachine(...));

as this makes the conversion explicit. Does this make sense, or did I miss something more?

awarzynski added inline comments.Apr 13 2022, 3:16 AM
flang/lib/Frontend/FrontendActions.cpp
491

Does this make sense, or did I miss something more?

Yes, I'm the one who missed something here :) Many thanks for pointing this out!

I convinced myself that I checked this, but clearly I didn't. And I confused the copy-assignment with a proper constructor. Every day is a school day!

I'll merge this shortly and I will incorporate your suggestion. Thanks for your patience going over this with me :)

ekieri added inline comments.Apr 13 2022, 8:16 AM
flang/lib/Frontend/FrontendActions.cpp
491

Well, thank you! I also learned something new.