Page MenuHomePhabricator

[flang][driver] Add support for `-c` and `-emit-obj`
ClosedPublic

Authored by awarzynski on Dec 15 2020, 7:59 AM.

Details

Summary

This patch adds a frontend action for emitting object files. While Flang
does not support code-generation, this action remains a placeholder.
This patch simply provides glue-code to connect the compiler driver
with the appropriate frontend action.

The new action is triggered with the -c compiler driver flag, i.e.
flang-new -c. This is then translated to flang-new -fc1 -emit-obj,
so -emit-obj has to be marked as supported as well.

As code-generation is not available yet, flang-new -c results in a
driver error:

error: code-generation is not available yet

Hopefully this will help communicating the level of available
functionality within Flang.

The definition of emit-obj is updated so that it can be shared between
Clang and Flang. As the original definition was enclosed within a
Clang-specific TableGen let statement, it is extracted into a new let
statement. That felt like the cleanest option.

I also commented out -triple in Flang::ConstructJob and updated some
comments there. This is similar to https://reviews.llvm.org/D93027. I
wanted to make sure that it's clear that we can't support -triple
until we have code-generation. However, once code-generation is
available we _will need_ -triple.

As this patch adds -emit-obj, the emit-obj.f90 becomes irrelevant and
is deleted. Instead, phases.f90 is added to demonstrate that users can
control compilation phases (indeed, -c is a phase control flag).

Depends on: D92854

Diff Detail

Event Timeline

awarzynski created this revision.Dec 15 2020, 7:59 AM
awarzynski requested review of this revision.Dec 15 2020, 7:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2020, 7:59 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

@dang , I would appreciate if you could take a look at the diff in Options.td - you introduced the most recent changes there: https://reviews.llvm.org/D82574. I got a bit confused with the nested let statements there. I couldn't find a better way of marking -emit-obj as a both CC1Option and FC1Option, so I just extracted it out.

awarzynski added a project: Restricted Project.Dec 15 2020, 9:22 AM
clementval added inline comments.
clang/include/clang/Driver/Options.td
5228

Is it intended to modify the SYCL options?

flang/lib/Frontend/CompilerInvocation.cpp
106–107

Should you remove this line since you have the case for it?

flang/test/Flang-Driver/code-gen.f90
12

than instead of then?

Thanks! for the patch, some minor comments inlined. Rest LGTM. Let others also have a look.

clang/include/clang/Driver/Options.td
4857

Please correct if I've misunderstood this change ?
You're removing this option from here(clang) and defining again at line 4631 as a common option to both clang flang ?
+1 to that.
However this seems out of the purview of this patch. Do you think having this as a separate patch(with specific intent) would be good(For tracking/isolating changes) ?
Or the least you can do is convey this intent in this patch Summary too.
I'm happy with either of those :)

flang/test/Flang-Driver/code-gen.f90
16

Since it's an error NOT a string(or similar) generated, I would rather have ERROR as a string check. This way it is self-evident for end reader.
Sort of:

! ERROR: code-generation is not available yet
awarzynski edited the summary of this revision. (Show Details)Dec 17 2020, 4:59 AM
awarzynski marked an inline comment as done.Dec 17 2020, 5:16 AM

@clementval & @SouraVX - thank you for your comments! I will upload an updated version shortly.

clang/include/clang/Driver/Options.td
4857

You're removing this option from here(clang) and defining again at line 4631 as a common option to both clang flang ?

Correct :)

However this seems out of the purview of this patch.

Not quite. Without this change you will get an error when using -emit-obj:

bin/flang-new -fc1 -emit-obj file.f90
error: unknown argument: '-emit-obj'

That's because it wouldn't be parsed here:
https://github.com/llvm/llvm-project/blob/eba09a2db9eab46832cb7ec7ef0d2c227747772a/flang/lib/Frontend/CompilerInvocation.cpp#L166-L167
(note the definition of includedFlagsBitmask).

I do agree that the Summary should be updated, thanks for pointing that out!

5228

This is just updating the comment (so it's just an nfc, SYCL options are not affected).

This closing } corresponds to the opening { specified here:: https://github.com/llvm/llvm-project/blob/eba09a2db9eab46832cb7ec7ef0d2c227747772a/clang/include/clang/Driver/Options.td#L4040. So, currently we have this:

let Flags = [CC1Option, NoDriverOption] in {
// A lot of code HERE
} // let Flags = [CC1Option]

Instead it should be:

let Flags = [CC1Option, NoDriverOption] in {
// A lot of code HERE
} // let Flags = [CC1Option, NoDriverOption]

I was mislead by this comment while working on this patch, so to me it seemed like a _related_ change :) But it isn't and should be extracted into a separate patch: https://github.com/llvm/llvm-project/commit/7f19712a6a9e65bdc9a9843ea488030bc12f3acc.

flang/lib/Frontend/CompilerInvocation.cpp
106–107

Yes, thanks for pointing out!

flang/test/Flang-Driver/code-gen.f90
16

Yeah, ERROR would be better. I wish that we had a prospect of Clang's -verify.

Address PR comments

This revision is now accepted and ready to land.Jan 5 2021, 8:47 AM
SouraVX accepted this revision.Jan 5 2021, 10:09 PM

LGTM! - Thanks!

This revision was landed with ongoing or failed builds.Jan 7 2021, 2:53 AM
This revision was automatically updated to reflect the committed changes.