This is an archive of the discontinued LLVM Phabricator instance.

Flang implementation for COMPILER_VERSION and COMPILER_OPTIONS intrinsics
ClosedPublic

Authored by hussainjk on Dec 21 2022, 9:33 PM.

Details

Summary

This revision implements the Fortran intrinsic procedures COMPILER_VERSION and COMPILER_OPTIONS from the iso_fortran_env module.
To be able to set the COMPILER_OPTIONS string according to the original compiler driver invocation, a string is passed to the frontend driver using the environment variable FLANG_COMPILER_OPTIONS_STRING, for lack of a better mechanism.

Fixes #59233

Diff Detail

Event Timeline

hussainjk created this revision.Dec 21 2022, 9:33 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
hussainjk requested review of this revision.Dec 21 2022, 9:33 PM

There's something wonky about this patch. I was unable to apply it against the latest sources. Can you please rebase?

ktras added a comment.Jan 4 2023, 10:22 AM

Is there a test you could add to this patch to show that the changes work as expected?

klausler added inline comments.
flang/include/flang/Common/Version.h
23

Don't indent the contents of a namespace.

Use capitalized names.

flang/lib/Lower/IntrinsicCall.cpp
747 ↗(On Diff #484735)

This intrinsic function must be folded during semantics, not implemented in code generation.

rouson added inline comments.Jan 30 2023, 11:54 AM
flang/lib/Lower/IntrinsicCall.cpp
747 ↗(On Diff #484735)

@klausler could you explain how one knows whether an intrinsic function must be folded during semantics or implemented in code generation?

klausler added inline comments.Jan 30 2023, 12:12 PM
flang/lib/Lower/IntrinsicCall.cpp
747 ↗(On Diff #484735)

F'2018 subclause "10.1.12 Constant expression" reads:

A constant expression is an expression with limitations that make it suitable for use as a kind type parameter, initializer, or named constant. It is an expression in which each operation is intrinsic, and each primary is ...

... (6) a reference to a standard intrinsic function that is transformational, other than COMMAND_ARGUMENT_COUNT, GET_TEAM, NULL, NUM_IMAGES, TEAM_NUMBER, THIS_IMAGE, or TRANSFER, where each argument is a constant expression,

16.10.2.7 COMPILER_VERSION ( )
1 Description. Processor-dependent string identifying the program translation phase.
2 Class. Transformational function.
3 Argument. None.

A reference to COMPILER_VERSION() is a primary that is a reference to a standard intrinsic transformational function other than COMMAND_ARGUMENT_COUNT, GET_TEAM, NULL, NUM_IMAGES, TEAM_NUMBER, THIS_IMAGE, or TRANSFER, and it has no argument that is not a constant expression, so it satisfies the requirements of point (6) in the definition of a constant expression, and so it is a constant expression.

hussainjk updated this revision to Diff 509597.Mar 30 2023, 3:37 AM

Re-implemented compiler_version, and implemented compiler_options, using intrinsic folding pass.

Please add a test.

flang/include/flang/Frontend/CompilerInvocation.h
86

This doesn't feel right. One of the task of the frontend driver is to parse command line arguments and split them into various fields in this class. There should be no dedicated field for ALL arguments (on top of other fields).

flang/lib/Frontend/CompilerInvocation.cpp
1052

Wouldn't this forward all command line arguments to set_compilerOptionsString?

ktras added inline comments.Mar 30 2023, 1:50 PM
flang/lib/Evaluate/intrinsics.cpp
2156–2157

Please delete this line of commented out code.

Rebasing and formatting.

hussainjk added inline comments.Apr 2 2023, 10:43 PM
flang/lib/Frontend/CompilerInvocation.cpp
1052

Pretty much. It wasn't clear to me to what extent should compiler_options be reporting command-line flags. Should it be more narrow?

The summary states "Implementation for COMPILER_VERSION intrinsic procedure", but it looks like this patch implements COMPILER_VERSION and COMPILER_OPTIONS. Please, could you update it accordingly? Also, please add the missing tests.

flang/lib/Frontend/CompilerInvocation.cpp
1052

I think that we are referring to two different things:

  • IIUC, you are referring to COMPILER_OPTIONS Fortran intrinsic,
  • I am referring to the design of the Flang driver and various implementation details, e.g. the fact that that the CompilerInvocation encapsulates a compiler invocation and that it groups compilation options into various sets, e.g. [[ flang/lib/Frontend/CompilerInvocation.cpp | parserOpts ]].

Please add a comment to explain that all compiler options are forwarded to the semantics context in order to implement COMPILER_OPTIONS. Also, rather than duplicating compiler options in commandLineArgs, please use the existing fields in CompilerInvocaiton.

hussainjk updated this revision to Diff 516430.Apr 24 2023, 9:03 AM

Adding test for compiler_version.

hussainjk added inline comments.Apr 24 2023, 9:17 AM
flang/lib/Frontend/CompilerInvocation.cpp
1052

So I did some tinkering around regarding this. The problem with the compiler option groupings is that they are encapsulated into a few different nonstandard classes, for which there is no method to iterate through, and which don't all have methods to retrieve a command-line string representation. So setting the compiler options string would then involve a lot of lines manually going through struct fields and appending option strings and corresponding values, which is a lot more fragile than just storing the original string. It seems like the current way should be preferred for maintainability.

Thanks for this patch. This patch fixes the following issue. If you can add `Fixes #59233 then the issue will automatically be closed when you submit.
https://github.com/llvm/llvm-project/issues/59233
You can consider using the test in the issue.

flang/lib/Frontend/CompilerInvocation.cpp
1052

It is not clear to me what options should be made visible to the user. Is it the options provided in the command line or the internal options that are used?

The current handling in the frontend driver captures the internal options and not the original options entered by the user. If we want the command line options then the compiler driver has this information and should pass it to the frontend driver as a string.

For eg: Currently, the following output is produced, which is not what the user provided.

$ ./bin/flang-new -flang-experimental-exec -O3 co.f90                                                                           
$ ./a.out 
 -triple aarch64-unknown-linux-gnu -emit-obj -fcolor-diagnostics -mrelocation-model pic -pic-level 2 -pic-is-pie -target-cpu generic -target-feature +neon -target-feature +v8a -O3 -o /tmp/co-cd57e2.o -x f95-cpp-input co.f90

I see that other compilers also provide some internal options but fewer.

$ gfortran -O3 co.f90 
$ ./a.out 
 -mlittle-endian -mabi=lp64 -O3 -fpre-include=/usr/include/finclude/math-vector-fortran.h
$ nvfortran -O3 co.f90 
$ ./a.out 
 co.f90 -O3 -Mvect=simd -Mflushz -Mcache_align -Mrecip-div -Mfactorize
flang/test/Evaluate/compiler_version.f90
2

We generally do not have emit-llvm tests here.

You can test this with either -fdebug-unparse or -fdebug-dump-parse-tree tests.

Please add a test for both compiler_version and compiler_options.

tschuett added inline comments.
flang/include/flang/Common/Version.h
23

Subversion should be Git.

30

Upstream LLVM is a monorepo. I don't believe that having flang and llvm in different repositories is supported.

klausler added inline comments.Apr 25 2023, 8:02 AM
flang/include/flang/Common/Version.h
30

Out-of-tree builds work just fine.

hussainjk updated this revision to Diff 522891.May 16 2023, 9:27 PM

Rewriting compiler_version test per review comments.

hussainjk updated this revision to Diff 523245.May 17 2023, 7:57 PM

Minor typo fix.

hussainjk edited the summary of this revision. (Show Details)May 17 2023, 7:58 PM
hussainjk updated this revision to Diff 523534.May 18 2023, 1:26 PM

Re-implemented compiler_options to be based on the options passed in by the user. Added a test for compiler_options.

hussainjk updated this revision to Diff 523818.May 19 2023, 9:16 AM

fixing build bugs.

hussainjk added inline comments.May 19 2023, 10:22 AM
flang/lib/Frontend/CompilerInvocation.cpp
1052

I rewrote this so that the compiler options string is instead set to the original command line invocation made by the user.
This information needs to be passed from the parent frontend process to the internal cc1 process which is subsequently launched. Currently it is doing this by setting an environment variable, but it could also be done with a command-line option passed to the cc1 stage, if that would be preferred.

Right now it is not including any internal expanded options, but that should be easy to do if we identify some that should be.

flang/lib/Frontend/CompilerInvocation.cpp
882

Not sure whether this style of using environment variables in the Driver to pass options between the Flang driver and the Flang frontend driver is OK. I was hoping you will pass it as an option with a string value to the Flang frontend driver.
-fc1 -co "<OPTIONS>"``

Please wait for @awarzynski for an opinion.

1052

Thanks @hussainjk. See comment inline regarding using environment variables for passing info.

@hussainjk , thanks for the updates!

The idea of using an environment variable to pass something from flang-new to flang-new -fc1 bypasses the "separation of concerns" in the driver design:

  • flang-new drives the frontend driver (flang-new -fc1), assembler and linker (i.e. creates invocations of these tools).
  • Once invoked, the frontend driver (flang-new -fc1) has no access to the compiler driver invocation - it doesn't need to. Indeed, why would the frontend be concerned with e.g. linker flags?

Passing it as a string, as @kiranchandramohan suggested, would help avoid requiring env variables. However, it would pollute the frontend driver invocation (flang-new -fc1 <args>) that people may want to use for debugging. I wish that we could avoid both, but I have no better suggestions. IMHO, this is inherently a Fortran issue. I am not a language expert, but do feel that COMPILER_OPTIONS is trying to expose information that belongs in the toolchain rather than language implementation.

Personally I am leaning towards accepting the approach with env variables. @kiranchandramohan, would that work? @hussainjk , could you add comments discouraging people from using similar mechanism for other things? Hopefully this is the only exception that we need to make.

Also, @hussainjk, your effort is much appreciated, thank you for experimenting with different approaches!

flang/lib/Frontend/CompilerInvocation.cpp
882
883–884

Please document that in one case the compiler will use "compiler driver" options (flang-new) and in the other "frontend driver" options (flang-new -fc1).

flang/test/Evaluate/compiler_options.f90
2

Similar test without -fc1 would be helpful too. Also, could you verify that with flang-new the user will only see the user specified options (so no flang-new -fc1 options)?

flang/tools/flang-driver/fc1_main.cpp
53–54

This change is no longer required, right?

hussainjk updated this revision to Diff 524153.May 21 2023, 8:58 PM

Addressing some review comments.

hussainjk added inline comments.May 21 2023, 9:08 PM
flang/tools/flang-driver/fc1_main.cpp
53–54

In the case when the frontend driver is invoked directly (and hence no env var), createFromArgs will set the COMPILER_OPTIONS string based on the actual command line, but it needs to be passed argv0 because it otherwise only gets the rest of argv.

hussainjk added inline comments.May 21 2023, 9:23 PM
flang/test/Evaluate/compiler_options.f90
2

It seems that -fc1 is necessary for -fdebug-unparse. So I could do this but using -emit-llvm. Thoughts, @kiranchandramohan?

@awarzynski thanks! I was also concerned about polluting the frontend invocation, as well as the scope creep of adding new command line option definitions.

Thanks again for the updates. This is heading in the right direction, but I'd like to see the documentation for introducing FLANG_COMPILER_OPTIONS_STRING and compilerOptionsString a bit more refined. See my suggestions inline.

flang/include/flang/Frontend/CompilerInvocation.h
85

Naming is hard and I don't claim my suggestion is perfect. My main point is that there's no need to include variable type in variable's name ;-)

Please also add a comment that explains what this string is for.

flang/lib/Frontend/CompilerInvocation.cpp
880
881–883

I think that this comment is missing some detail. Please see my suggestion and feel free to re-use and/or edit.

flang/test/Evaluate/compiler_options.f90
2

You can add a test with -emit-llvm in flang/test/Driver. Just document that that would be specifically to test communicating COMPILER_OPTIONS from flang-new to flang-new -fc1.

7

?

flang/tools/flang-driver/driver.cpp
140–141

Please be more specific.

  1. I am aware of COMPILER_OPTIONS Fortran intrinsic and compilerOptionsString string in Flang's CompilerInvocation.h (that I suggested to be renamed). Which one is it?
  2. Why is this env variable being passed to the frontend driver like this? Please document that this is specifically for the COMPILER_OPTIONS intrinsic and that there is no other mechanism to communicate the original invocation between the compiler and the frontend drivers.

We are making an exception here to avoid excessive polution of the frontend invocation.

Not quite. An exception is being made, because we haven't been able to identify a better alternative to implement COMPILER_OPTIONS intrinsics. We could, instead, add an additional option to the frontend driver invocation, but that wouldn't be 100% problem-free.

How about:

Set the environment variable, FLANG_COMPILER_OPTIONS_STRING, to contain all the compiler options. This is intended for the frontend driver, flang-new -fc1, to enable the implementation of the COMPILER_OPTIONS intrinsic. To this end, the frontend driver requires the list of the original compiler options, which is not available through other means.
TODO: This way of passing information between the compiler and frontend drivers is discouraged. We should find a better way not involving env variables.

146

IMHO, compilerOptsAsOneString would be more descriptive. In either way, please adhere to the coding style and use camelCase.

flang/tools/flang-driver/fc1_main.cpp
53–54
awarzynski added inline comments.May 28 2023, 4:58 AM
flang/lib/Frontend/CompilerInvocation.cpp
820

Clang and other tools use response files when the command line gets too long, e.g.
https://reviews.llvm.org/D4897#change-Me6ZMDruqhhS

Note the @

hussainjk updated this revision to Diff 526501.May 29 2023, 8:19 PM

Addressing review comments.

hussainjk updated this revision to Diff 526625.May 30 2023, 8:00 AM

Addressing review comments.

awarzynski accepted this revision.May 30 2023, 8:30 AM

The driver logic LGTM, thank you for addressing my comments (I've added one more formatting suggestion)! Please wait a day or two before landing this - I've only really looked at the bits I'm familiar with (i.e. the driver).

Also, could you update the summary/commit message. I would appreciate if you could "introduce" the rationale for adding the env variable. Thank you!

flang/include/flang/Frontend/CompilerInvocation.h
85–87
This revision is now accepted and ready to land.May 30 2023, 8:30 AM
hussainjk retitled this revision from Implementation for COMPILER_VERSION intrinsic procedure to Flang implementation for COMPILER_VERSION and COMPILER_OPTIONS intrinsics.Jun 1 2023, 9:09 AM
hussainjk edited the summary of this revision. (Show Details)

The out-of-tree build is now broken and there are a number of style problems with a new source file; see #general on Slack. Please address the out-of-tree build failure in a timely manner.

sscalpone added inline comments.Jun 3 2023, 4:47 AM
flang/test/Driver/compiler_options.f90
12 ↗(On Diff #527460)

Why close?

The out-of-tree build bot is still failing.

Scratch that; the out-of-tree build bot has now built successfully with your latest commit. Thank you for that.

Please clean up the other issues that I enumerated on Slack.