This is an archive of the discontinued LLVM Phabricator instance.

[flang][driver] Add support for -save-temps
ClosedPublic

Authored by awarzynski on Apr 29 2022, 5:07 AM.

Details

Summary

This patch adds support for -save-temps in flang-new, Flang's
compiler driver. The semantics of this option are inherited from Clang.

The file extension for temporary Fortran preprocessed files is set to
i. This is identical to what Clang uses for C (or C++) preprocessed
files. I have tried researching what other compilers do here, but I
couldn't find any definitive answers. One GFortran thread [1] suggests
that indeed it is not clear what the right approach should be.

Normally, various phases in Clang/Flang are combined. The -save-temps
option works by forcing the compiler to run every phase separately. As
there is no integrated assembler driver in Flang, user will have to use
-save-temps together with -fno-integrated-as. Otherwise, an
invocation to the integrated assembler would be generated generated,
which is going to fail (i.e. something equivalent to clang -cc1as from
Clang).

There are no specific plans for implementing an integrated assembler for
Flang for now. One possible solution would be to share it entirely with
Clang.

[1] https://gcc.gnu.org/bugzilla//show_bug.cgi?id=81615

Depends on D124664
Depends on D124665
Depends on D124667

Diff Detail

Event Timeline

awarzynski created this revision.Apr 29 2022, 5:07 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
awarzynski requested review of this revision.Apr 29 2022, 5:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2022, 5:07 AM
schweitz added inline comments.Apr 29 2022, 11:19 AM
clang/include/clang/Driver/Options.td
3989

nit: I noticed that this is getting rid of spaces after commas on a few lines. Is there any sense of a prior style being used in that regard?

unterumarmung added inline comments.Apr 29 2022, 12:24 PM
clang/include/clang/Driver/Options.td
4131

Why not to add FC1Option here and elsewhere like it's done for CC1Option?

awarzynski added inline comments.May 1 2022, 8:51 AM
clang/include/clang/Driver/Options.td
3989

I swear that somebody recently requested that I remove a space in one of my patches. I've tried finding it and failed ... I must have dreamed it!

Thanks Eric, I'll revert this change :)

4131

I'm not 100% sure what -fno-integrated-as controls in Clang's frontend driver, clang -cc1. I'm guessing that it might related to using/not-using LLVM's MCAsmParser. Perhaps for inline assembly?

In Flang, I'm only focusing on -save-temps for which I need to make sure that we don't try to call flang-new -fc1as or something similar (it does not exist). Instead, -save-temps will have to rely on an external assembler.

So, we basically don't require -fno-integrated-as in flang-new -fc1 just yet (that's what FC1Option is for - marking an option as available in the frontend driver).

rovka added inline comments.May 2 2022, 1:42 AM
clang/include/clang/Driver/Options.td
4131

I'm not 100% sure either (haven't looked at the code), but my understanding of -fno-integrated-as is that it forces clang to call the system assembler as opposed to using the LLVM libraries to generate machine code directly. Since flang never uses the system assembler, I would say the integrated assembler is always on in flang. So I'm not convinced it makes sense to add this flag to the driver, unless we're actually adding a
-fc1as.

unterumarmung added inline comments.May 2 2022, 9:41 AM
clang/include/clang/Driver/Options.td
4131

@awarzynski @rovka, thank you for the detailed explanation!

rovka added a comment.May 3 2022, 1:08 AM

I think I confused myself yesterday, it does make sense to add -fno-integrated-as for this. Could we add a test for it independent of save-temps?

awarzynski updated this revision to Diff 426739.May 3 2022, 9:10 AM

Add a test, restore white-space in Options.td

awarzynski updated this revision to Diff 426747.May 3 2022, 9:32 AM

Fix failing test

rovka added inline comments.May 4 2022, 1:18 AM
flang/test/Driver/fno-integrated-as.f90
7
11
19

Nit (here and above): Is it a rule that -o blah comes right before the input file, or should we also add a {{.*}} in between in case other flags sneak in?

flang/test/Driver/save-temps.f90
2
awarzynski updated this revision to Diff 427036.May 4 2022, 9:05 AM

Fix typos as per comments from @rovka, thanks!

awarzynski marked 4 inline comments as done.May 4 2022, 9:05 AM
awarzynski added inline comments.
flang/test/Driver/fno-integrated-as.f90
19

Nit (here and above): Is it a rule that -o blah comes right before the input file, or should we also add a {{.*}} in between in case other flags sneak in?

TBH, it's not something that I checked, but it's been the case so far :) We may need to insert {{.*}} there at some point, but perhaps lets delay until it's really needed? This way we will know exactly what violates that rule and perhaps we could tweak then?

rovka accepted this revision.May 5 2022, 5:45 AM

LGTM, thanks! It would be nice to rebase the patch and see the pre-commit CI passing, but then again you're the one dealing with the buildbots if you break anything, so do as you prefer :)

This revision is now accepted and ready to land.May 5 2022, 5:45 AM
awarzynski updated this revision to Diff 427333.May 5 2022, 8:27 AM
awarzynski marked an inline comment as done.

Rebase on top of main

awarzynski updated this revision to Diff 427367.May 5 2022, 9:54 AM

Mark the tests as unsupported on Windows

Pre-merge testing is failing on Windows. Here is the error message:

flang-new: error: there is no external assembler that can be used on this platform

I will update the commit message accordingly before merging this.

Pre-merge CI is 🍏 . If there are no new comments, I'd like to merge this tomorrow.

schweitz accepted this revision.May 5 2022, 12:43 PM
This revision was automatically updated to reflect the committed changes.