[PATCH] D145845: [Flang] Allow compile *.f03, *.f08 file

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

I'd move these tests to flang/test/Driver/supported-suffices. That would help 
documenting what these tests actual check for. Also, one could be tempted to 
test for other suffices and then it would be nice to keep them in one place. Ta!




Comment at: flang/test/Driver/f03-suffix.f03:1
+! RUN: %flang -### -flang-experimental-exec %s 2>&1 | FileCheck %s
+

You can safely drop `-flang-experimental-exec` here.



Comment at: flang/test/Driver/f03-suffix.f03:4
+! CHECK: "{{.*}}flang-new" "-fc1" {{.*}} "/tmp/{{.*}}.o"
+! CHECK: "{{.*}}ld" {{.*}} "/tmp/{{.*}}.o"
+program f03

The linker invocation is unrelated to this change. Also, it will very likely 
look differently on other operating systems. Best to skip it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145845

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145845: [Flang] Allow compile *.f03, *.f08 file

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D145845#4186608 , @clementval 
wrote:

> In D145845#4186607 , @awarzynski 
> wrote:
>
>> Thanks!
>>
>> In D145845#4186590 , @clementval 
>> wrote:
>>
>>> Thanks. Can you add tests in `flang/test/Driver`?
>>
>> Hm, this is a `clangDriver` change rather than anything Flang specific 樂 . 
>> Btw, how does https://github.com/llvm/llvm-project/issues/61260 manifest to 
>> you? I'm trying to figure out how to best test it (I couldn't find any other 
>> tests for this specifically).
>
> Cannot we just have a a .f03 file and compile it and see if we have an 
> executable?

We don't really build executable for testing the Driver. Also, building an 
executable involves many steps:

1. The driver (`flang-new`) identifies the file as a valid Fortran file. 
Delegates to "Flang" for the next step.
2. "Flang" parses the input, runs semantic checks and lowers to LLVM IR. This 
is driven by the frontend driver, `flang-new -fc1`.
3. LLVM IR is compiled into machine code (via LLVM) (also driven by `flang-new 
-fc1`).
4. Machine code generated in 3 is then linked by the driver to generate an 
executable (this step is driven by `flang-new`).

  It would be good to understand what exactly this change unblocks and then 
test that specifically. To me it sounds like this change is only really needed 
for 1. and hence we should only be testing 1 and avoid doing extra work that's 
not needed.

I suspect that without this change, `.*.f03` files are recognized as object 
rather than Fortran files and hence step 1. is followed by step 4. rather than 
step 2. So, try `flang-new -### file.f03` **with** and **without** your change. 
You should see that **with** this change you do see the frontend driver 
invocation  (`flang-new -fc1 `) that would run steps 2 and 3.. IMO 
that's the only thing that requires verifying here.

Does this make sense?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145845

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145845: [Flang] Allow compile *.f03, *.f08 file

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks!

In D145845#4186590 , @clementval 
wrote:

> Thanks. Can you add tests in `flang/test/Driver`?

Hm, this is a `clangDriver` change rather than anything Flang specific 樂 . Btw, 
how does https://github.com/llvm/llvm-project/issues/61260 manifest to you? I'm 
trying to figure out how to best test it (I couldn't find any other tests for 
this specifically).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145845

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144864: [Flang][Driver][MLIR] Add -fopenmp-is-device to Flang and link to an omp.is_device attribute

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski accepted this revision.
awarzynski added a comment.

In D144864#4175001 , @agozillon wrote:

> And then provided @awarzynski is happy with the current state of the patch

LGTM, thanks for the updates and for working on this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144864

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144864: [Flang][Driver][MLIR] Add -fopenmp-is-device to Flang and link to an omp.is_device attribute

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: clang/test/Driver/flang/flang-omp.f90:1
+! Check that flang -fc1 is invoked when in --driver-mode=flang 
+! and the relevant openmp and openmp offload flags are utilised

agozillon wrote:
> awarzynski wrote:
> > agozillon wrote:
> > > awarzynski wrote:
> > > > agozillon wrote:
> > > > > agozillon wrote:
> > > > > > awarzynski wrote:
> > > > > > > This test looks correct to me, but please note that:
> > > > > > > 
> > > > > > > ```
> > > > > > > ! Check that flang -fc1 is invoked when in --driver-mode=flang 
> > > > > > > ```
> > > > > > > 
> > > > > > > yet (`%clang` instead of `%flang`)
> > > > > > > 
> > > > > > > ```
> > > > > > > ! RUN: %clang --driver-mode=flang -### -fopenmp %s 2>&1 | 
> > > > > > > FileCheck --check-prefixes=CHECK-OPENMP %s
> > > > > > > ```
> > > > > > > 
> > > > > > > I'm not really sure whether we should be testing Flang-specific 
> > > > > > > logic in Clang. Having said that, Flang does use `clangDriver` to 
> > > > > > > implement its driver :) 
> > > > > > > 
> > > > > > > You could consider using 
> > > > > > > https://github.com/llvm/llvm-project/blob/main/flang/test/Driver/frontend-forwarding.f90
> > > > > > >  instead (or add an OpenMP specific file there).
> > > > > > > 
> > > > > > > Not a blocker.
> > > > > > Yes, I wasn't so sure either as it felt a little weird to test 
> > > > > > Flang components inside of Clang, but as you said it is a Clang 
> > > > > > toolchain (that this test is checking) and borrows from the 
> > > > > > clangDriver! 
> > > > > > 
> > > > > > I borrowed this test from other similar tests in the same folder 
> > > > > > that test other flang specific driver logic in a similar manner, 
> > > > > > but I am more than happy to add an additional flang specific driver 
> > > > > > test as you mention! 
> > > > > On further looking into the frontend-forwarding test, I don't know if 
> > > > > it is suitable for fopenmp-is-device as it is an fc1 option and won't 
> > > > > be forwarded from the flang-new frontend down to fc1 at the moment! 
> > > > > 
> > > > > I think this test will be more suitable when additional flags like 
> > > > > the fopenmp-targets (or similar flags) that are used in this test are 
> > > > > added to the Flang driver. As they spawn/propagate the 
> > > > > openmp-is-device flag. However, perhaps I am incorrect.
> > > > > On further looking into the frontend-forwarding test, I don't know if 
> > > > > it is suitable for fopenmp-is-device as it is an fc1 option and won't 
> > > > > be forwarded from the flang-new frontend down to fc1 at the moment!
> > > > 
> > > > It should be - that's what the logic in Flang.cpp does, no? And if it 
> > > > doesn't, is it a deliberate design decision?
> > > I believe the logic in Flang.h/Flang.cpp just invokes for -fc1 not the 
> > > Frontend driver, at least that's what it looks like from the ConstructJob 
> > > task that Clang invokes. 
> > > 
> > > It's a deliberate design decision that -fopenmp-is-device is an fc1 
> > > option, it mimics the way Clang currently does it, which is a cc1 option, 
> > > there is no way to directly specify it to clang without -cc1 I think. The 
> > > hope is to keep the Flang OpenMP flags as similar to Clang's as possible 
> > > for the time being I believe.
> > > I believe the logic in Flang.h/Flang.cpp just invokes for -fc1 not the 
> > > Frontend driver.
> > 
> > `flang-new -fc1` **is** the frontend driver ;-) So:
> > * you've added the  logic to forward `-fopenmp-is-device` that should work 
> > for both `flang-new` and `clang --driver-mode=flang`, but
> > * you're only testing one of these.
> > 
> > There should have tests for both. In particular for `flang-new` given that 
> > this patch adds new functionality to LLVM Flang. If that doesn't work, 
> > let's try to figure it out together.
> > 
> > > The hope is to keep the Flang OpenMP flags as similar to Clang's as 
> > > possible for the time being I believe.
> > 
> > +1 We should try as much as we can to keep Clang's and Flang's behavior 
> > consistent :) (which you do  )
> Ah I didn't realise that, thank you very much for clarifying that for me! :) 
> I thought it was bypassing it with -fc1, and yjsy flang-new without it was 
> the frontend! I still have a lot to learn, so I apologies for the 
> misunderstanding! 
> 
> In this case do you mean test with "flang-new -fc1 -fopenmp-is-device" or 
> "flang-new -fopenmp-is-device", as the latter will not be accepted by 
> flang-new in the current patch but I do test the -fc1 variation in the 
> omp-is-device.f90 test which is more about testing the attribute admittedly 
> as opposed to the driver command. 
> 
> I don't know if testing it in the frontend-forwarding.f90 test is the right 
> location in this case as it doesn't test with -fc1, it seems to test options 
> are forwarded to -fc1 correctly, I could be incorrect though. In future 
> passing an argument like 

[PATCH] D144864: [Flang][Driver][MLIR] Add -fopenmp-is-device to Flang and link to an omp.is_device attribute

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: clang/test/Driver/flang/flang-omp.f90:1
+! Check that flang -fc1 is invoked when in --driver-mode=flang 
+! and the relevant openmp and openmp offload flags are utilised

agozillon wrote:
> awarzynski wrote:
> > agozillon wrote:
> > > agozillon wrote:
> > > > awarzynski wrote:
> > > > > This test looks correct to me, but please note that:
> > > > > 
> > > > > ```
> > > > > ! Check that flang -fc1 is invoked when in --driver-mode=flang 
> > > > > ```
> > > > > 
> > > > > yet (`%clang` instead of `%flang`)
> > > > > 
> > > > > ```
> > > > > ! RUN: %clang --driver-mode=flang -### -fopenmp %s 2>&1 | FileCheck 
> > > > > --check-prefixes=CHECK-OPENMP %s
> > > > > ```
> > > > > 
> > > > > I'm not really sure whether we should be testing Flang-specific logic 
> > > > > in Clang. Having said that, Flang does use `clangDriver` to implement 
> > > > > its driver :) 
> > > > > 
> > > > > You could consider using 
> > > > > https://github.com/llvm/llvm-project/blob/main/flang/test/Driver/frontend-forwarding.f90
> > > > >  instead (or add an OpenMP specific file there).
> > > > > 
> > > > > Not a blocker.
> > > > Yes, I wasn't so sure either as it felt a little weird to test Flang 
> > > > components inside of Clang, but as you said it is a Clang toolchain 
> > > > (that this test is checking) and borrows from the clangDriver! 
> > > > 
> > > > I borrowed this test from other similar tests in the same folder that 
> > > > test other flang specific driver logic in a similar manner, but I am 
> > > > more than happy to add an additional flang specific driver test as you 
> > > > mention! 
> > > On further looking into the frontend-forwarding test, I don't know if it 
> > > is suitable for fopenmp-is-device as it is an fc1 option and won't be 
> > > forwarded from the flang-new frontend down to fc1 at the moment! 
> > > 
> > > I think this test will be more suitable when additional flags like the 
> > > fopenmp-targets (or similar flags) that are used in this test are added 
> > > to the Flang driver. As they spawn/propagate the openmp-is-device flag. 
> > > However, perhaps I am incorrect.
> > > On further looking into the frontend-forwarding test, I don't know if it 
> > > is suitable for fopenmp-is-device as it is an fc1 option and won't be 
> > > forwarded from the flang-new frontend down to fc1 at the moment!
> > 
> > It should be - that's what the logic in Flang.cpp does, no? And if it 
> > doesn't, is it a deliberate design decision?
> I believe the logic in Flang.h/Flang.cpp just invokes for -fc1 not the 
> Frontend driver, at least that's what it looks like from the ConstructJob 
> task that Clang invokes. 
> 
> It's a deliberate design decision that -fopenmp-is-device is an fc1 option, 
> it mimics the way Clang currently does it, which is a cc1 option, there is no 
> way to directly specify it to clang without -cc1 I think. The hope is to keep 
> the Flang OpenMP flags as similar to Clang's as possible for the time being I 
> believe.
> I believe the logic in Flang.h/Flang.cpp just invokes for -fc1 not the 
> Frontend driver.

`flang-new -fc1` **is** the frontend driver ;-) So:
* you've added the  logic to forward `-fopenmp-is-device` that should work for 
both `flang-new` and `clang --driver-mode=flang`, but
* you're only testing one of these.

There should have tests for both. In particular for `flang-new` given that this 
patch adds new functionality to LLVM Flang. If that doesn't work, let's try to 
figure it out together.

> The hope is to keep the Flang OpenMP flags as similar to Clang's as possible 
> for the time being I believe.

+1 We should try as much as we can to keep Clang's and Flang's behavior 
consistent :) (which you do  )


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144864

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144864: [Flang][Driver][MLIR] Add -fopenmp-is-device to Flang and link to an omp.is_device attribute

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: clang/test/Driver/flang/flang-omp.f90:1
+! Check that flang -fc1 is invoked when in --driver-mode=flang 
+! and the relevant openmp and openmp offload flags are utilised

agozillon wrote:
> agozillon wrote:
> > awarzynski wrote:
> > > This test looks correct to me, but please note that:
> > > 
> > > ```
> > > ! Check that flang -fc1 is invoked when in --driver-mode=flang 
> > > ```
> > > 
> > > yet (`%clang` instead of `%flang`)
> > > 
> > > ```
> > > ! RUN: %clang --driver-mode=flang -### -fopenmp %s 2>&1 | FileCheck 
> > > --check-prefixes=CHECK-OPENMP %s
> > > ```
> > > 
> > > I'm not really sure whether we should be testing Flang-specific logic in 
> > > Clang. Having said that, Flang does use `clangDriver` to implement its 
> > > driver :) 
> > > 
> > > You could consider using 
> > > https://github.com/llvm/llvm-project/blob/main/flang/test/Driver/frontend-forwarding.f90
> > >  instead (or add an OpenMP specific file there).
> > > 
> > > Not a blocker.
> > Yes, I wasn't so sure either as it felt a little weird to test Flang 
> > components inside of Clang, but as you said it is a Clang toolchain (that 
> > this test is checking) and borrows from the clangDriver! 
> > 
> > I borrowed this test from other similar tests in the same folder that test 
> > other flang specific driver logic in a similar manner, but I am more than 
> > happy to add an additional flang specific driver test as you mention! 
> On further looking into the frontend-forwarding test, I don't know if it is 
> suitable for fopenmp-is-device as it is an fc1 option and won't be forwarded 
> from the flang-new frontend down to fc1 at the moment! 
> 
> I think this test will be more suitable when additional flags like the 
> fopenmp-targets (or similar flags) that are used in this test are added to 
> the Flang driver. As they spawn/propagate the openmp-is-device flag. However, 
> perhaps I am incorrect.
> On further looking into the frontend-forwarding test, I don't know if it is 
> suitable for fopenmp-is-device as it is an fc1 option and won't be forwarded 
> from the flang-new frontend down to fc1 at the moment!

It should be - that's what the logic in Flang.cpp does, no? And if it doesn't, 
is it a deliberate design decision?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144864

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144864: [Flang][Driver][MLIR] Add -fopenmp-is-device to Flang and link to an omp.is_device attribute

[Andrzej Warzynski via Phabricator via cfe-commits]

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

> Thank you very much @awarzynski for suggesting them that was a great help.

I'm happy that I could help :) The driver logic LGTM, but please wait for 
either @jdoerfert and/or @kiranchandramohan to also approve. Thanks for 
contributing!




Comment at: clang/test/Driver/flang/flang-omp.f90:1
+! Check that flang -fc1 is invoked when in --driver-mode=flang 
+! and the relevant openmp and openmp offload flags are utilised

This test looks correct to me, but please note that:

```
! Check that flang -fc1 is invoked when in --driver-mode=flang 
```

yet (`%clang` instead of `%flang`)

```
! RUN: %clang --driver-mode=flang -### -fopenmp %s 2>&1 | FileCheck 
--check-prefixes=CHECK-OPENMP %s
```

I'm not really sure whether we should be testing Flang-specific logic in Clang. 
Having said that, Flang does use `clangDriver` to implement its driver :) 

You could consider using 
https://github.com/llvm/llvm-project/blob/main/flang/test/Driver/frontend-forwarding.f90
 instead (or add an OpenMP specific file there).

Not a blocker.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144864

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144864: [Flang][Driver][MLIR] Add -fopenmp-is-device to Flang and link to an omp.is_device attribute

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:121-125
+  if (IsOpenMPDevice) {
+// -fopenmp-is-device is passed along to tell the frontend that it is
+// generating code for a device, so that only the relevant declarations are
+// emitted.
+CmdArgs.push_back("-fopenmp-is-device");

Can you add a test for this section?



Comment at: flang/test/Lower/OpenMP/omp-is-device.f90:1
+!RUN: %flang_fc1 -emit-fir -fopenmp -fopenmp-is-device %s -o - | FileCheck %s 
--check-prefix=DEVICE
+!RUN: %flang_fc1 -emit-fir -fopenmp %s -o - | FileCheck %s --check-prefix=HOST

What happens if `-fopenmp-is-device` is used without `-fopnemp`?



Comment at: flang/tools/bbc/bbc.cpp:127
+static llvm::cl::opt
+enableOpenMPDevice("fopenmp-is-device",
+   llvm::cl::desc("enable openmp device compilation"),

This option is not tested


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144864

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143704: [Flang] Part one of Feature List action

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D143704#4150680 , @jdoerfert wrote:

> can we have a test?

+1




Comment at: flang/examples/FeatureList/FeatureList.cpp:34
+
+struct ASTVisitor {
+private:

There is no AST in Flang - could you propose something more descriptive?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143704

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143301: [flang] Handle unsupported warning flags

[Andrzej Warzynski via Phabricator via cfe-commits]

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

LGTM, thanks for seeing this through! :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143301

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143301: [flang] Handle unsupported warning flags

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: clang/include/clang/Driver/Options.td:5045
 
+// Unsupported flang W options
+defm : FlangIgnoredDiagOpt<"extra">;

The name of the sub-project is "Flang", the sub-dir is "flang" and the driver 
name is "flang-new". What is "flang" referring to in this comment?

Also, "-W options" would be IMHO a bit more descriptive.



Comment at: clang/include/clang/Driver/Options.td:5047
+defm : FlangIgnoredDiagOpt<"extra">;
+defm : FlangIgnoredDiagOpt<"aliasing">;
+defm : FlangIgnoredDiagOpt<"ampersand">;

How about a test for all the options that follow `-Wextra`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143301

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143301: [flang] Handle unsupported warning flags

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

@elmcdonough , let me rephrase this (should've been clearer before, sorry):

> One thing that's not clear to me - how come "-Wextra" is not treated as an 
> error and "-Wblah" is?

Where's the logic that makes sure that `-Wextra` (and other flags that you 
redefine here) is reported as unused? That's not clear from this definition:

  multiclass FlangIgnoredDiagOpt {
def unsupported_warning_w#NAME : Flag<["-", "--"], "W"#name>, 
Group;
  }

In particular, I don't see anything that would check whether a particular 
option is in this group: `flang_ignored_w_Group`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143301

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143301: [flang] Handle unsupported warning flags

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for the update! One thing that's not clear to me - how come "-Wextra" 
**is not** treated as an error and "-Wblah" **is**? That's not clear from the 
code.

I'm also realising that I incorrectly assumed that `-Wextra` was defined in 
Options.td. Instead, it's defined in DiagnosticGroup.td 
.
 I've not worked with that file and it's not clear to me how it works. IIUC, 
that file encapsulates all the user-facing logic controlling diagnostics in 
Clang. I am mentioning this as in your approach you are effectively re-defining 
"-Wextra" in Options.td. That's unfortunate (it's duplication), but IMO the 
approach taken here is fine - all tests pass and tweaking Clang's 
DiagnosticGroup.td is beyond the scope. I just want understand what "makes this 
work" :)




Comment at: clang/include/clang/Driver/Options.td:4881
 
+multiclass FlangIgnoredWarning {
+  def unsupported_warning_w#NAME : Flag<["-", "--"], "W"#name>, 
Group;

[nit] These are diag options.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143301

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143301: Emit warning for unsupported gfortran flags

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D143301#4126884 , @elmcdonough 
wrote:

> Thanks for the feedback everyone.  This is my current understanding on what I 
> should do: I am to rename gfortran_unsupported_Group to flang_ignored_w_Group 
> and move the non W group gfortran options into another patch.  I have these 
> changes locally and am currently building + testing.  Please let me know if 
> I'm misinterpreting these instructions.

Makes sense and sorry for confusion. Just to clarify, "w" in ` 
flang_ignored_w_Group` stands for "warning" :) I think that having two separate 
patches will simplify the discussion too ;-) And, most importantly, thanks for 
doing this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143301

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143301: Emit warning for unsupported gfortran flags

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D143301#4126855 , @jdoerfert wrote:

> In D143301#4126712 , @awarzynski 
> wrote:
>
>>> I think the -W stuff can go in, it has tests and is reasonable.
>>
>> I'd like for us to rely on a flag from Options.td for this instead. 
>> Something similar to clang_ignored_f_Group 
>> .
>>  I would probably call it `flang_ignored_w_Group` :)
>
> For the -W stuff? You want to remove the explicit warning then (which is 
> generally fine too)?

I had something like this in mind: 
https://github.com/llvm/llvm-project/blob/7301a7ce196e217c077b2b68f58366be48664223/clang/lib/Driver/ToolChains/Clang.cpp#L7448.
 Also, the whole logic could be moved to the compiler driver (i.e. Flang.cpp) 樂 
. Emitting a warning makes sense, but do we care about the frontend driver 
(i.e. "CompilerInvocation.cpp")? (which is intended for developers familiar 
with the implementation?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143301

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143301: Emit warning for unsupported gfortran flags

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D143301#4126682 , @jdoerfert wrote:

> Split this into two patches/reviews.

+1

> I think the -W stuff can go in, it has tests and is reasonable.

I'd like for us to rely on a flag from Options.td for this instead. Something 
similar to clang_ignored_f_Group 
.
 I would probably call it `flang_ignored_w_Group` :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143301

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143493: [flang][driver] Add support for -include flag in flang -fc1

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D143493#4110272 , @skatrak wrote:

> It is introduced to the arguments list in `Clang::AddPreprocessingOptions` in 
> certain cases to add the OpenMP wrapper "__clang_openmp_device_functions.h" 
> to the list of includes for device offload.

This is a use case in Clang ;-)

> It seemed likely that we would need to add a similar mechanism eventually to 
> flang as well

Possibly, but it's not needed yet?

I would rather refrain from adding flags to Flang only because a similar flag 
exists in Clang. I suggest abandoning this until there's a clear need for this 
in Flang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143493

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143493: [flang][driver] Add support for -include flag in flang -fc1

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In what cases would this flag be used in practice? I've scanned Clang and 
couldn't find any answers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143493

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143478: [RFC][Flang][driver] Try to support `flang -fc1as`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for working on this! Before diving any deeper

- Does LLVM Flang require `flang-new -fc1as`?
- If yes, why can't Flang and Clang share it?

For better visibility, I suggest raising these questions on Discourse 
. As this could potentially affect both Flang and 
Clang, could you post in one of the generic sections? Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143478

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140795: [Flang] Add user option -funderscoring/-fnounderscoring to control trailing underscore added to external names

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Driver changes LGTM, thanks! I will defer to others for changes in other areas.


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

https://reviews.llvm.org/D140795

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140972: [flang] Add -fstack-arrays flag

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski accepted this revision.
awarzynski added a comment.

Thanks for the updates, LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140972

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140972: [flang] Add -fstack-arrays flag

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: clang/include/clang/Driver/Options.td:485-486
 
+// Works like BoolOption except without specifying a KeyPathAndMacro, as these
+// would refer to non-existant members of clang data structures
+multiclass BoolFlangOnlyOptionhttps://reviews.llvm.org/D140972/new/

https://reviews.llvm.org/D140972

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140972: [flang] Add -fstack-arrays flag

[Andrzej Warzynski via Phabricator via cfe-commits]

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

LGTM, thanks!




Comment at: flang/test/Transforms/stack-arrays.f90:22-29
+! LLVM-IR: array_value_copy_simple
+! LLVM-IR-NOT: malloc
+! LLVM-IR-NOT: free
+! LLVM-IR: alloca [4 x i32]
+! LLVM-IR-NOT: malloc
+! LLVM-IR-NOT: free
+! LLVM-IR: ret void

Hopefully I got this right, it's been a while :) 
https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-dag-directive


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140972

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140972: [flang] Add -fstack-arrays flag

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/include/flang/Tools/CLOptions.inc:159
 inline void createDefaultFIROptimizerPassPipeline(
-mlir::PassManager , llvm::OptimizationLevel optLevel = defaultOptLevel) 
{
+mlir::PassManager , bool stackArrays = false, llvm::OptimizationLevel 
optLevel = defaultOptLevel) {
   // simplify the IR

tblah wrote:
> awarzynski wrote:
> > CLANG-FORMAT-ME :) Same for other long lines here.
> It seems clang-format is not run on this file. I have fixed the lines changed 
> in my patch. Should I clang-format the whole file in a separate patch?
+1



Comment at: flang/test/Transforms/stack-arrays.f90:3
 
+! We have to check llvm ir here to force flang to run the whole mlir pipeline
+! this is just to check that -fstack-arrays enables the stack-arrays pass so

tblah wrote:
> awarzynski wrote:
> > Also, I don't quite follow this comment:
> > 
> > >  We have to check llvm ir here to force flang to run the whole mlir 
> > > pipeline
> > 
> > Why is checking LLVM IR going to force Flang to run anything?
> Just running `flang-new -fc1 -emit-fir` outputs the FIR before all of the 
> transformation passes run (which is why I have to pipe the result through 
> `fir-opt` to run the array value copy and stack arrays passes on the first 
> line).
> 
> Outputting LLVM IR requires all of the MLIR transformation passes to be run 
> to transform all of the higher level dialects into the LLVM dialect. Stack 
> arrays is run as part of that same pipeline 
> (`fir::createMLIRToLLVMPassPipeline`). I want to test that `-fstack-arrays` 
> does cause the stack arrays pass to run as part of that pipeline, which is 
> why I am checking the LLVM-IR.
> 
> One alternative would be to print out which passes have run and check that, 
> but I felt this way fits more with the spirit of the other tests.
Thanks for the explanation. So you want to verify this functionality by 
investigating two pipelines:
* source --> FIR
* source --> LLVM IR
You could convey that by using `CHECK-FIR` and `CHECK-LLVM-IR` (or just `FIR` 
and `LLVM-IR`). I "complained", because this is unclear:
> We have to check llvm ir here to force flang to run the whole mlir pipeline
Perhaps:
> In order to verify the whole MLIR pipeline, make the driver generate LLVM IR.



Comment at: flang/test/Transforms/stack-arrays.f90:6
+! only check the first example
+! RUN: %flang_fc1 -emit-llvm -o - -fstack-arrays %s | FileCheck 
--check-prefix=CHECK-LLVM %s
+

tblah wrote:
> awarzynski wrote:
> > 
> Lots of other tests do not follow this convention. It would change the 
> FileCheck comments to look like `! LLVM: array_value_copy_simple`. 
> 
> I think it is good to require `CHECK-FEATURE:` so that any mention of 
> `FEATURE` in comments does not confuse FileCheck. Especially for a name like 
> LLVM which is likely to be written in capitals.
> Lots of other tests do not follow this convention.

I think that we would easily find examples for either approach.

> I think it is good to require CHECK-FEATURE:

Well, `LLVM` is not a feature ;-) Also, most folks working with LLVM are 
familiar with the LIT syntax - the presence of `CHECK` in `CHECK-FEATURE` is 
just unnecessary noise to me. But I am happy either way. But I would appreciate 
replacing with `LLVM` with something a bit more meaningful - perhaps `LLVM-IR`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140972

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140972: [flang] Add -fstack-arrays flag

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.
Herald added a subscriber: sunshaoce.



Comment at: clang/include/clang/Driver/Options.td:5056-5060
+def fstack_arrays : Flag<["-"], "fstack-arrays">, Group,
+  HelpText<"Attempt to allocate array temporaries on the stack, no matter 
their size">;
+def fno_stack_arrays : Flag<["-"], "fno-stack-arrays">, Group,
+  HelpText<"Allocate array temporaries on the heap (default)">;
+

We should avoid duplicating options like this and use multiclasses instead. For 
example, see how [[ 
https://github.com/llvm/llvm-project/blob/b6ceadf3b663427f3cc233bbfdb5e35017cabd9e/clang/include/clang/Driver/Options.td#L6464-L6467
 | debug_pass_manager ]] is defined.



Comment at: flang/docs/FlangDriver.md:573
+`-O3 -ffast-math -fstack-arrays -fno-semantic-interposition`.
 `-fno-semantic-interposition` is not used because clang does not enable this as
 part of `-Ofast` as the default behaviour is similar.

[nit] "Clang" is a sub-project. It's not clear what "clang" would be - also a 
sub-project or the binary?



Comment at: flang/include/flang/Tools/CLOptions.inc:159
 inline void createDefaultFIROptimizerPassPipeline(
-mlir::PassManager , llvm::OptimizationLevel optLevel = defaultOptLevel) 
{
+mlir::PassManager , bool stackArrays = false, llvm::OptimizationLevel 
optLevel = defaultOptLevel) {
   // simplify the IR

CLANG-FORMAT-ME :) Same for other long lines here.



Comment at: flang/include/flang/Tools/CLOptions.inc:216
 inline void createMLIRToLLVMPassPipeline(
-mlir::PassManager , llvm::OptimizationLevel optLevel = defaultOptLevel) 
{
+mlir::PassManager , bool stackArrays = false, llvm::OptimizationLevel 
optLevel = defaultOptLevel) {
   // Add default optimizer pass pipeline.

[nit] To me, it would make more sense to put `stackArrays` at the end. 
`optLevel`is a more powerful flag. 



Comment at: flang/test/Transforms/stack-arrays.f90:3
 
+! We have to check llvm ir here to force flang to run the whole mlir pipeline
+! this is just to check that -fstack-arrays enables the stack-arrays pass so

Also, I don't quite follow this comment:

>  We have to check llvm ir here to force flang to run the whole mlir pipeline

Why is checking LLVM IR going to force Flang to run anything?



Comment at: flang/test/Transforms/stack-arrays.f90:6
+! only check the first example
+! RUN: %flang_fc1 -emit-llvm -o - -fstack-arrays %s | FileCheck 
--check-prefix=CHECK-LLVM %s
+





Comment at: flang/tools/bbc/bbc.cpp:276
 // Add O2 optimizer pass pipeline.
-fir::createDefaultFIROptimizerPassPipeline(pm, 
llvm::OptimizationLevel::O2);
+fir::createDefaultFIROptimizerPassPipeline(pm, false,
+   llvm::OptimizationLevel::O2);

Similar suggestion below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140972

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140795: [Flang] Add user option -funderscoring/-fnounderscoring to enable/disable ExternalNameConversionPass

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D140795#4031392 , @kkwli0 wrote:

> The purpose of this option is to control the trailing underscore being 
> appended to external names (e.g. procedure names, common block names). The 
> option in gfortran is documented in 
> https://gcc.gnu.org/onlinedocs/gfortran/Code-Gen-Options.html.

Thanks for this explanation - much appreciated! Could this short description be 
added to the summary? Also,  could you add a note stating whether the semantics 
of this option in Flang are consistent with GFortran. Ta!

> However, I don't think the patch does what we want. Given a procedure name 
> `foo`, the `-fno-underscoring` option will give `_QPfoo` instead of `foo`. We 
> will look into it.

Names in Flang are mangled at the FIR level. I couldn't find any documentation 
for this, but the ExternalNameConversion 

 pass could be helpful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140795

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140795: [Flang] Add user option -funderscoring/-fnounderscoring to enable/disable ExternalNameConversionPass

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Hi @madanial , thanks for posting this!

> This patch adds user option -funderscoring/-fnounderscoring which behaves 
> similar to the gfortran option be enabling/disabling the 
> ExternalNameConversionPass

I don't quite understand what this option is for and it's hard to deduce from 
the patch. Please, could you add a link to some documentation? And tests.

-Andrzej


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140795

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137995: [Flang][Driver] Handle target CPU and features

[Andrzej Warzynski via Phabricator via cfe-commits]

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

LGTM, thank you!


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

https://reviews.llvm.org/D137995

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137995: [Flang][Driver] Handle target CPU and features

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for all the updates @mnadeem! Mostly looks good. A few more nits, but 
nothing substantial :)

In D137995#3958824 , @mnadeem wrote:

> In D137995#3931005 , 
> @kiranchandramohan wrote:
>
>> We might need `-fc1` tests as well.
>
> What kind of tests do you think would be appropriate here? Can you point me 
> to any examples, maybe from clang?

I didn't see any tests in Clang specifically for the frontend driver flags taht 
are added here (`i.e. `-target-cpu` and `-target-feature`). However, you could 
add a test for situations like this: `-target-cpu=my-imaginary-cpu` and 
`-target-fature=my-amazing-non-existent-feature`.




Comment at: clang/lib/Driver/ToolChains/Flang.cpp:98
+  default:
+// Untested for other targets but should work generally.
+break;

[nit] I don't think that this comment contributes much. To me it is 
self-explanatory that only the triples that are actually present here are 
tested. Having said that, I don't mind keeping it here.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:100
+break;
+  case llvm::Triple::aarch64:
+  case llvm::Triple::x86_64:

nit



Comment at: flang/include/flang/Frontend/TargetOptions.h:25-30
 /// Options for controlling the target. Currently this is just a placeholder.
 /// In the future, we will use this to specify various target options that
 /// will affect the generated code e.g.:
 ///   * CPU to tune the code for
-///   * available CPU/hardware extensions
-///   * target specific features to enable/disable
 ///   * options for accelerators (e.g. GPUs)
 ///   * (...)

I think that we can trim this now. WDYT?



Comment at: flang/test/Driver/target-cpu-features.f90:25-26
+
+! CHECK-A57: "-triple" "aarch64-unknown-linux-gnu"
+! CHECK-A57: "-target-cpu" "cortex-a57" "-target-feature" "+v8a" 
"-target-feature" "+crc" "-target-feature" "+crypto" "-target-feature" 
"+fp-armv8" "-target-feature" "+neon" "-target-feature" "+sha2" 
"-target-feature" "+aes"
+

1. By adding `-fc1` you make it clear that it's the frontend driver invocation 
that's being tested.
2. `CHECK-SAME` makes sure the triple is matched with the right set of features.


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

https://reviews.llvm.org/D137995

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129156: Add -fpass-plugin option to Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D129156#3949156 , @ro wrote:

> This introduced a new failure on Solaris:
>
>   FAIL: Flang :: Driver/pass-plugin-not-found.f90
>
> Running the failing command manually shows:
>
>   error: unable to load plugin 'X.Y': 'Could not load library 'X.Y': ld.so.1: 
> flang-new: X.Y: open failed: No such file or directory'
>
> while the test currently expects
>
>   error: unable to load plugin 'X.Y': 'Could not load library 'X.Y': X.Y: 
> cannot open shared object file: No such file or directory'
>
> This expectation is unjustified, I believe: the message is produced by 
> `PassPlugin::Load` -> `sys::DynamicLibrary::getPermanentLibrary` -> 
> `HandleSet::DLOpen` -> `::dlerror`.  Obviously the output of the last is 
> system-dependent; I don't think we can put any requirements on it (maybe not 
> even the exact wording of the `No such file or directory` part.

Sorry that you are hitting this - things like this happen, sadly. I think that 
the easiest to resolve it would be to tweak the expected error so that it works 
on Solaris as well as other popular platforms. Given that you might be the only 
person able to test on Solaris, would you be able to upload something for  a 
review? Thanks!


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

https://reviews.llvm.org/D129156

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137995: [Flang][Driver] Handle target CPU and features

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for implementing this!

> Processes target cpu and features in the flang driver. Right now features are 
> only added for AArch64 because I only did basic testing on AArch64 but it 
> should generally work for others as well.

X86 is a very popular target and we have pre-commit CI as well. And X86 
buildbots :) Please include X86.

Question: are the option semantics identical that what you get in `clang -cc1`? 
If yes, could you add a comment in the summary?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137995

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129156: Add -fpass-plugin option to Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Please be aware of https://reviews.llvm.org/D137673 - you may need to rebase if 
it lands before this patch.

Please just go ahead and merge, but please keep `WIN32` in the final version of 
"flang/test/CMakeLists.txt" (see my comment inline).

LGTM




Comment at: flang/test/CMakeLists.txt:65
 )
+if (NOT WIN32)
+  list(APPEND FLANG_TEST_DEPENDS Bye)

awarzynski wrote:
> IIUC, `Bye` is only needed when plugins are enabled.
IIUC, `WIN32` is still required. Sorry for not being clearer earlier.


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

https://reviews.llvm.org/D129156

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137329: [flang] Add -f[no-]associative-math and -mreassociate

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D137329#3910249 , 
@kiranchandramohan wrote:

> In D137329#3909943 , @awarzynski 
> wrote:
>
>> In D137329#3909082 , @clementval 
>> wrote:
>>
>>> Wouldn't it be good to have a RFC for all these options and what they will 
>>> do in Flang instead of just adding them all? Or did I miss the RFC?
>>
>> +1
>
> Fast Math attributes supported in LLVM IR are documented in 
> https://llvm.org/docs/LangRef.html#fast-math-flags. This set of patches 
> (details below) provides a way to set or unset each of these attributes.
>
> While making policy decisions about these flags, like for ffp-contract, we 
> have created an RFC 
> (https://discourse.llvm.org/t/rfc-ffp-contract-default-value/66301).  When 
> adding umbrella flags like 
> -ffast-math/-Ofast/-ffp-model/-funsafe-math-optimizations, we will submit 
> RFCs.
>
> Patch : Flag Name : LLVM IR Attribute
> https://reviews.llvm.org/D137325 : f[no-]honor-nans: nnan
> https://reviews.llvm.org/D137072 : f[no-]honor-infinities  : ninf
> https://reviews.llvm.org/D137328 : f[no-]signed-zeros  : nsz
> https://reviews.llvm.org/D137330 : f[no-]reciprocal-math : arcp
> https://reviews.llvm.org/D136080 : ffp-contract option : contract
> https://reviews.llvm.org/D137326 : f[no-]approx-func: afn
> https://reviews.llvm.org/D137329 : f[no-]associative-math: reassoc

Thanks for this summary Kiran!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137329

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137329: [flang] Add -f[no-]associative-math and -mreassociate

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D137329#3909082 , @clementval 
wrote:

> Wouldn't it be good to have a RFC for all these options and what they will do 
> in Flang instead of just adding them all? Or did I miss the RFC?

+1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137329

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129156: Add -fpass-plugin option to Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/test/CMakeLists.txt:65
 )
+if (NOT WIN32)
+  list(APPEND FLANG_TEST_DEPENDS Bye)

IIUC, `Bye` is only needed when plugins are enabled.


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

https://reviews.llvm.org/D129156

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136080: [flang] Add -ffp-contract option processing

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski accepted this revision.
awarzynski added a comment.

LGTM, thanks for implementing this!




Comment at: clang/lib/Driver/ToolChains/Flang.cpp:98-99
+} else
+  // Clang's "fast-honor-pragmas" option is not supported because it is
+  // non-standard and pragmas are not relevant to Fortran.
+  D.Diag(diag::err_drv_unsupported_option_argument)

[nit] "... and pragmas are not relevant to Fortran." Fortran has directives 
rather than pragmas. So it's not quite like pragmas are not supported. It's 
just that there's different mechanism instead.


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

https://reviews.llvm.org/D136080

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136080: [flang] Add -ffp-contract option processing

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for all the updates, Tom! I have a few more suggestions.

From the summary:

> implement these pragmas

Could you explain what pragmas you are referring to here? (i.e. Clang pragmas 
for C and C++ + link)

> gfortran uses "fast" by default

For our future self, could you add a link as well?




Comment at: clang/include/clang/Driver/Options.td:1925
   " | fast-honor-pragmas (fuses across statements unless diectated by 
pragmas)."
-  " Default is 'fast' for CUDA, 'fast-honor-pragmas' for HIP, and 'on' 
otherwise.">,
+  " Default is 'fast' for CUDA, 'fast-honor-pragmas' for HIP, 'off' for flang, 
and 'on' otherwise.">,
+  HelpText<"Form fused FP ops (e.g. FMAs)">,

I still think that we shouldn't be making references to Flang in Clang 
documentation. And this `DocBrief` is only used by Clang. Also, "flang" is 
problematic - what do you mean by "flang"?



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:91-98
+} else if (Val.equals("fast-honor-pragmas")) {
+  D.Diag(diag::warn_drv_unsupported_option_for_flang)
+  << Val << A->getOption().getName() << "fast";
+  FPContract = "fast";
+} else if (Val.equals("on")) {
+  D.Diag(diag::warn_drv_unsupported_option_for_flang)
+  << Val << A->getOption().getName() << "off";

Some "unsupported" options are treated as errors and some are warnings. I think 
that for the sake of consistency it would be better to keep them all as errors. 
Also, why not use `Val` instead of e.g. "off"?


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

https://reviews.llvm.org/D136080

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136080: [flang] Add -ffp-contract option processing

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Added Clang reviewers who touched the definition of `--fp-contract` most 
recently. Mostly for visibility, but lets give them at least a couple of days 
to take a look at the changes in Options.td.


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

https://reviews.llvm.org/D136080

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136080: [flang] Add -ffp-contract option processing

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1926
+  " Default is 'fast' for CUDA, 'fast-honor-pragmas' for HIP, 'off' for flang, 
and 'on' otherwise.">,
+  HelpText<"Form fused FP ops (e.g. FMAs): fast | off (clang, flang), on | 
fast-honor-pragmas (clang only)">,
   Values<"fast,on,off,fast-honor-pragmas">;

As far as users are concerned, `flang-new` is just a Fortran compiler. Once we 
start adding references to Clang in `flang-new --help`, we might be confusing 
users (i.e. where's the boundary between Clang and Flang?) and exposing 
implementation details (i.e. that `flang-new` is implemented using 
`clangDriver`). Ideally we should avoid that.



Comment at: flang/include/flang/Frontend/LangOptions.h:29
+
+// Enable the floating point pragma
+FPM_On,

tblah wrote:
> vzakhari wrote:
> > tblah wrote:
> > > vzakhari wrote:
> > > > tblah wrote:
> > > > > awarzynski wrote:
> > > > > > What are these pragmas? Perhaps you can add a test that would 
> > > > > > include them?
> > > > > I copied this comment from clang. I believe the pragma is 
> > > > > ```
> > > > > #pragma clang fp contract(fast)
> > > > > ```
> > > > > 
> > > > > See 
> > > > > https://clang.llvm.org/docs/LanguageExtensions.html#extensions-to-specify-floating-point-flags
> > > > > 
> > > > > This patch only adds support for argument processing, so I can't test 
> > > > > for the pragmas.
> > > > I do not think we should blindly copy this from clang.  I believe 
> > > > `-ffp-contract=on`  is there to do the contraction complying to the 
> > > > language standard - but Fortran standard says nothing about 
> > > > contraction.  I am also not aware about a Fortran compiler that 
> > > > supports directives related to contraction, so `fast-honor-pragmas` 
> > > > does not seem to be applicable as well.  Basically, we end up with just 
> > > > `off` and `fast`.
> > > > 
> > > > Now, it may be reasonable to support the same `-ffp-contract=` 
> > > > arguments so that users can apply the same options sets for C/C++ and 
> > > > Fortran compilations.  If we want to do this, we need to map `on` and 
> > > > `fast-honor-pragmas` to something, e.g. `fast`.  A driver warning (not 
> > > > an error) may be useful to make the option's behavior clear when `on` 
> > > > and `fast-honor-pragmas` are passed.
> > > From the clang help text:
> > > ```
> > > Form fused FP ops (e.g. FMAs):
> > > fast (fuses across statements disregarding pragmas)
> > > | on (only fuses in the same statement unless dictated by pragmas)
> > > | off (never fuses)
> > > Default is 'on'
> > > ```
> > > 
> > > So if we removed `on` and set the default to `off` we would no longer 
> > > fuse within the same statement by default.
> > > 
> > > Classic-flang seems to support `on`, `off` and `fast`: 
> > > https://github.com/flang-compiler/flang/blob/master/test/llvm_ir_correct/fma.f90
> > Not talking about defaults just yet, I think Flang cannot currently support 
> > the `on` mode as documented.
> > 
> > I do not have the latest classic flang readily availalbe, but I am curious 
> > what it will generate for this example:
> > ```
> > function fn(x,y,z)
> >   real :: x,y,z
> >   fn = x * y
> >   fn = fn + z
> > end function
> > ```
> > 
> > With a very old classic flang I get just `fast` math flags on the multiple 
> > and add instructions, which is obviously not what `on` is supposed to do:
> > ```
> > $ flang -target aarch64-linux-gnu -O1 -c -S -emit-llvm -ffp-contract=on 
> > fma.f90
> > $ cat fma.ll
> >   %4 = fmul fast float %3, %1, !dbg !10
> >   %5 = bitcast i64* %z to float*, !dbg !11
> >   %6 = load float, float* %5, align 4, !dbg !11
> >   %7 = fadd fast float %6, %4, !dbg !11
> > ```
> > 
> > Maybe the latest classic flang does support it properly, e.g. it only 
> > contracts operations from the same statement.  But I do not see a way to 
> > support this in Flang right now, so documenting the `on` mode as it is in 
> > clang seems confusing.
> > 
> > We can still support `on` in the Flang option, but I think we need to issue 
> > a warning saying that it defaults to something else, e.g. to `fast`.  If 
> > mapping `on` to `fast` is not appropriate to some users, then they will 
> > have to explicitly specify `-ffp-contract=off` for Fortran compilations in 
> > their build system.
> > 
> > I am also curious what `fuses in the same statement` means for Fortran.  
> > For example:
> > ```
> >   x1 = DOT_PRODUCT(x2, x3)+x4*x5+x6
> > ```
> > 
> > If Fortran processor decides to implement `DOT_PRODUCT` as inline 
> > multiply+add loop, does `-ffp-contract=on` apply to them or it only applies 
> > to `x4*x5+x6`?
> Thanks for your feedback.
> 
> I've updated my patch. Now flang only supports `off` and `fast`. The other 
> two map to `fast` and we default to `off`.
> 
> gfortran seems to default to `fast`:
> ```
> -ffp-contract=style
> 
> -ffp-contract=off disables 

[PATCH] D136080: [flang] Add -ffp-contract option processing

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/test/Driver/driver-help-hidden.f90:34
 ! CHECK-NEXT: Use  as character line width in fixed mode
+! CHECK-NEXT: -ffp-contract= Form fused FP ops (e.g. FMAs): fast (fuses 
across statements disregarding pragmas) | on (only fuses in the same statement 
unless dictated by pragmas) | off (never fuses) | fast-honor-pragmas (fuses 
across statements unless diectated by pragmas). Default is 'fast' for CUDA, 
'fast-honor-pragmas' for HIP, and 'on' otherwise.
 ! CHECK-NEXT: -ffree-formProcess source files in free form

tblah wrote:
> vzakhari wrote:
> > Is it easy to emit a different help message for Flang to say that there are 
> > only two modes for Fortran?
> @awarzynski tells me there is no way to do this short of having separate 
> `Options.td` for flang and clang. Once we have settled on which arguments to 
> support, I will update the shared help text to mention flang.
Both `clang -help` and `flang-new -help` must be 100% correct. As this help 
text is not valid in the case of LLVM Flang, it needs to be updated 
accordingly. 

As @tblah points out, there's no straightforward mechanism for having a custom 
help texts for `clang` and `flang-new` in Clang's driver library ATM. But I 
think that this can be achieved even without creating a separate "Options.td" 
file. One would have to define a new tablegen record in "Options.td". That 
would be a separate patch though, probably accompanied by an RFC.

There's a different solution too. Note that currently the definition uses the 
`HelpText` field. However, you could use [[ 
https://github.com/llvm/llvm-project/blob/b1d7a95e4e4a2b57cbe02636bbe357dc48d615c5/clang/include/clang/Driver/Options.td#L81-L82
 | DocBrief ]] instead (which we used to solve a similar issue with [[ 
https://github.com/llvm/llvm-project/blob/b1d7a95e4e4a2b57cbe02636bbe357dc48d615c5/clang/include/clang/Driver/Options.td#L696-L704
 | -I ]]). That's what I suggest that you do. Basically, copy the contents of 
`HelpText` for `-ffp-contract` into a `DocBrief` field (we don't use this field 
in Flang and it should probably be renamed as `DocBriefClang`). `HelpText` 
should be replaced with something brief that applies both to Clang and Flang.

Btw, what's the help-text/spelling in `gfortran`?


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

https://reviews.llvm.org/D136080

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136080: [flang] Add -ffp-contract option processing

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

> The omission of the fast-honor-pragmas argument from the compiler driver is 
> deliberate.

Where is it omitted?

> I suspect the CI failure on windows is unrelated to my code

I agree.




Comment at: clang/lib/Driver/ToolChains/Flang.cpp:165
+  // Floating point related options
+  AddFloatingPointOptions(D, Args, CmdArgs);
+

From [[ 
https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
 | LLVM Coding style ]]

> Function names should be verb phrases (as they represent actions), and 
> command-like function should be imperative. The name should be camel case, 
> and start with a lower case letter (e.g. openFile() or isFoo()).

I know that this style is not followed here 100%. But that's what we should aim 
for :)



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:85-86
+ArgStringList ) {
+  // TODO: share RenderFloatingPointOptions from ./Clang.cpp and use that
+  // instead of duplicating code here
+  StringRef FPContract;

tblah wrote:
> awarzynski wrote:
> > What's RenderFloatingPointOptions?
> It is a static function in clang/lib/Driver/ToolChains/Clang.cpp which does 
> the same job as AddFloatingPointOptions, except for clang. I couldn't use it 
> right away because not all of the options it it processes are supported in 
> flang, but once we get there it would make sense to share code.
Ah! That [[ 
https://github.com/llvm/llvm-project/blob/b1d7a95e4e4a2b57cbe02636bbe357dc48d615c5/clang/lib/Driver/ToolChains/Clang.cpp#L2753-L3185
 | function ]] is over 400 LOC and looks super complex. I doubt Flang will be 
able to share that logic with Clang any time soon. If ever. I would actually 
skip this comment. Sometimes code duplication is the right approach ;-)



Comment at: flang/include/flang/Frontend/LangOptions.h:9-11
+//  This file defines the CodeGenOptions interface, which holds the
+//  configuration for LLVM's middle-end and back-end. It controls LLVM's code
+//  generation into assembly or machine code.

UPDATEME



Comment at: flang/include/flang/Frontend/LangOptions.h:29
+
+// Enable the floating point pragma
+FPM_On,

What are these pragmas? Perhaps you can add a test that would include them?



Comment at: flang/include/flang/Frontend/LangOptions.h:49-50
+
+/// Tracks various options which control the dialect of fortran that is
+/// accepted. Based on clang::LangOptions
+class LangOptions : public LangOptionsBase {





Comment at: flang/lib/Frontend/CompilerInvocation.cpp:661-662
 
+/// Parses all floating point related arguments and populates the variables
+/// options accordingly. Returns false if new errors are generated.
+///

> populates the variables options accordingly

Perhaps this would be a bit more specific/descriptive:

"and configures the input CompilerInvocation accordingly". Ultimately, this is 
about ... configuring the current compiler invocation, which is represented by 
an instance of `CompilerInvocaiton`. 



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:693
+
+// TODO: actually use the flag in codegen
+diags.Report(diagUnimplemented) << a->getOption().getName();

In here you only configure `CompilerInvocation`. This configuration will be 
used elsewhere (i.e. where codegen happens). So, I think, as far as this method 
is concerned the implementation is complete.



Comment at: flang/test/Driver/flang_fp_opts.f90:1-2
+! Test for passing of floating point options between the compiler and frontend
+! drivers.
+

No longer valid ;-)



Comment at: flang/test/Driver/flang_fp_opts.f90:4
+
+! RUN: %flang_fc1 -ffp-contract=fast %s 2>&1 | FileCheck %s
+! CHECK: ffp-contract= is not currently implemented

Can you test with `flang` as well?


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

https://reviews.llvm.org/D136080

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136080: [flang] Add -ffp-contract option processing

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for implementing this, @tblah!

Two high level questions/requests:

- are you confident that we will need LangOptions.def?
- can you upload a patch with full context? (some details can be found here: 
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface)

-Andrzej




Comment at: clang/lib/Driver/ToolChains/Flang.cpp:85-86
+ArgStringList ) {
+  // TODO: share RenderFloatingPointOptions from ./Clang.cpp and use that
+  // instead of duplicating code here
+  StringRef FPContract;

What's RenderFloatingPointOptions?



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:664
+///
+/// \param [out] res Stores the processed arguments
+/// \param [in] args The arguments to parse

`res` is a very confusing name (that I used myself in various places). 
Basically, it's the `CompilerInvocation` instance ... result. Perhaps use 
`invoc`? 



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:665
+/// \param [out] res Stores the processed arguments
+/// \param [in] args The arguments to parse
+/// \param [out] diags DiagnosticsEngine to report erros with

[nit] "The compiler invocation args to parse"



Comment at: flang/test/Driver/driver-help.f90:108
 ! HELP-FC1-NEXT: Use  as character line width in fixed mode
+! HELP-FC1-NEXT: -ffp-contract= Form fused FP ops (e.g. FMAs): fast 
(fuses across statements disregarding pragmas) | on (only fuses in the same 
statement unless dictated by pragmas) | off (never fuses) | fast-honor-pragmas 
(fuses across statements unless diectated by pragmas). Default is 'fast' for 
CUDA, 'fast-honor-pragmas' for HIP, and 'on' otherwise.
 ! HELP-FC1-NEXT: -ffree-formProcess source files in free form

Why not expose this flag in `flang-new`? (as well as `flang-new -fc1`?)



Comment at: flang/test/Driver/flang_fp_opts.f90:1-2
+! Test for passing of floating point options between the compiler and frontend
+! drivers.
+

Sounds like a test for [[ 
https://github.com/llvm/llvm-project/blob/main/flang/test/Driver/frontend-forwarding.f90
 | frontend-forwarding.f90 ]]



Comment at: flang/test/Driver/flang_fp_opts.f90:6-7
+
+! CHECK1-LABEL: "-fc1"
+! CHECK1: -ffp-contract=fast
+

Could you use more descriptive prefixes?


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

https://reviews.llvm.org/D136080

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130078: [flang][nfc] Rename `AddOtherOptions` as `ForwardOptions`

[Andrzej Warzynski via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG174e954e370e: [flang][nfc] Rename `AddOtherOptions` as 
`ForwardOptions` (authored by awarzynski).

Changed prior to commit:
  https://reviews.llvm.org/D130078?vs=447250=467798#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130078

Files:
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/ToolChains/Flang.h


Index: clang/lib/Driver/ToolChains/Flang.h
===
--- clang/lib/Driver/ToolChains/Flang.h
+++ clang/lib/Driver/ToolChains/Flang.h
@@ -29,7 +29,7 @@
   ///
   /// \param [in] Args The list of input driver arguments
   /// \param [out] CmdArgs The list of output command arguments
-  void AddFortranDialectOptions(const llvm::opt::ArgList ,
+  void addFortranDialectOptions(const llvm::opt::ArgList ,
 llvm::opt::ArgStringList ) const;
 
   /// Extract preprocessing options from the driver arguments and add them to
@@ -37,7 +37,7 @@
   ///
   /// \param [in] Args The list of input driver arguments
   /// \param [out] CmdArgs The list of output command arguments
-  void AddPreprocessingOptions(const llvm::opt::ArgList ,
+  void addPreprocessingOptions(const llvm::opt::ArgList ,
llvm::opt::ArgStringList ) const;
 
   /// Extract PIC options from the driver arguments and add them to
@@ -48,13 +48,12 @@
   void AddPicOptions(const llvm::opt::ArgList ,
  llvm::opt::ArgStringList ) const;
 
-  /// Extract other compilation options from the driver arguments and add them
-  /// to the command arguments.
+  /// This method will effectively copy options from \a Args into \a CmdArgs.
   ///
   /// \param [in] Args The list of input driver arguments
   /// \param [out] CmdArgs The list of output command arguments
-  void AddOtherOptions(const llvm::opt::ArgList ,
-   llvm::opt::ArgStringList ) const;
+  void forwardOptions(const llvm::opt::ArgList ,
+  llvm::opt::ArgStringList ) const;
 
 public:
   Flang(const ToolChain );
Index: clang/lib/Driver/ToolChains/Flang.cpp
===
--- clang/lib/Driver/ToolChains/Flang.cpp
+++ clang/lib/Driver/ToolChains/Flang.cpp
@@ -27,7 +27,7 @@
   CmdArgs.push_back(types::getTypeName(Input.getType()));
 }
 
-void Flang::AddFortranDialectOptions(const ArgList ,
+void Flang::addFortranDialectOptions(const ArgList ,
  ArgStringList ) const {
   Args.AddAllArgs(
   CmdArgs, {options::OPT_ffixed_form, options::OPT_ffree_form,
@@ -44,14 +44,14 @@
 options::OPT_fno_automatic});
 }
 
-void Flang::AddPreprocessingOptions(const ArgList ,
+void Flang::addPreprocessingOptions(const ArgList ,
 ArgStringList ) const {
   Args.AddAllArgs(CmdArgs,
   {options::OPT_P, options::OPT_D, options::OPT_U,
options::OPT_I, options::OPT_cpp, options::OPT_nocpp});
 }
 
-void Flang::AddOtherOptions(const ArgList , ArgStringList ) const 
{
+void Flang::forwardOptions(const ArgList , ArgStringList ) const {
   Args.AddAllArgs(CmdArgs,
   {options::OPT_module_dir, options::OPT_fdebug_module_writer,
options::OPT_fintrinsic_modules_path, options::OPT_pedantic,
@@ -127,9 +127,9 @@
   // Add preprocessing options like -I, -D, etc. if we are using the
   // preprocessor (i.e. skip when dealing with e.g. binary files).
   if (types::getPreprocessedType(InputType) != types::TY_INVALID)
-AddPreprocessingOptions(Args, CmdArgs);
+addPreprocessingOptions(Args, CmdArgs);
 
-  AddFortranDialectOptions(Args, CmdArgs);
+  addFortranDialectOptions(Args, CmdArgs);
 
   // Color diagnostics are parsed by the driver directly from argv and later
   // re-parsed to construct this job; claim any possible color diagnostic here
@@ -142,8 +142,8 @@
   // -fPIC and related options.
   AddPicOptions(Args, CmdArgs);
 
-  // Add other compile options
-  AddOtherOptions(Args, CmdArgs);
+  // Handle options which are simply forwarded to -fc1.
+  forwardOptions(Args, CmdArgs);
 
   // Forward -Xflang arguments to -fc1
   Args.AddAllArgValues(CmdArgs, options::OPT_Xflang);


Index: clang/lib/Driver/ToolChains/Flang.h
===
--- clang/lib/Driver/ToolChains/Flang.h
+++ clang/lib/Driver/ToolChains/Flang.h
@@ -29,7 +29,7 @@
   ///
   /// \param [in] Args The list of input driver arguments
   /// \param [out] CmdArgs The list of output command arguments
-  void AddFortranDialectOptions(const llvm::opt::ArgList ,
+  void addFortranDialectOptions(const llvm::opt::ArgList ,
 llvm::opt::ArgStringList ) const;
 
   

[PATCH] D125788: [flang][driver] Rename `flang-new` as `flang`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski abandoned this revision.
awarzynski added a comment.

In D125788#3622274 , @clementval 
wrote:

> There are open discussion so wait for other to confirm or not.

I was under the impression that we did discuss this extensively in our 
community calls and agreed to proceed with it. At least that's how I recall it.

I am abandoning this change as I'm distracted with other tasks at the moment, 
but I am still strongly in favor of this. Please, feel free to commandeer this 
patch and continue the discussion. I will try my best to help with reviews.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125788

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129156: Add -fpass-plugin option to Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D129156#3853045 , @tarunprabhu 
wrote:

> The tests still passed.

No, it wasn't run. We need to understand why before proceeding.


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

https://reviews.llvm.org/D129156

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski accepted this revision.
awarzynski added a comment.

The driver looks good, thanks for all the effort!


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129156: Add -fpass-plugin option to Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D129156#3852553 , @tarunprabhu 
wrote:

> In D129156#3851843 , @awarzynski 
> wrote:
>
>> @tarunprabhu Could you confirm that with the build command above 
>> "pass-plugin.f90" is failing for you? It is for me.
>
> @awarzynski What compiler do you use to build this? gcc doesn't seem to like 
> `-gmlt`, clang-12 complains about `-fuse-ld=lld` (could be the clang 
> installation on the machine on which I am building), and clang-13 causes a 
> compile error.
>
> [EDIT: Tag @awarzynski]

I doubt those particular flags matter here. You can trim that CMake command as 
follows:

  cmake ../llvm -D LLVM_ENABLE_PROJECTS="mlir;flang;clang;llvm" -G Ninja -D 
CMAKE_BUILD_TYPE=Release -D LLVM_ENABLE_ASSERTIONS=ON -D LLVM_BUILD_EXAMPLES=ON 
  ninja check-flang


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

https://reviews.llvm.org/D129156

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129156: Add -fpass-plugin option to Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

@tarunprabhu Could you confirm that with the build command above 
"pass-plugin.f90" is failing for you? It is for me.

In order to fix that, you will have to add this 

 in Flang's test/CMakeLists.txt 
.
 After the change, `libBye.so` will be added as a dependency to Flang tests, 
but only when `FLANG_BUILD_EXAMPLES` is set. I would also add a note in 
"pass-plugin.f90" that it depends on the "Bye" pass from LLVM.


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

https://reviews.llvm.org/D129156

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129156: Add -fpass-plugin option to Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In @clementval 's CMake invocation `LLVM_BUILD_EXAMPLES` is not set, which 
means that the examples (e.g. the `Bye` plugin) are not built. Adding  `! 
REQUIRES: examples` to the test should fix the failure in this case. I did 
verify locally.

However, the pre-commit CI that was failing did include the examples:

  cmake ../llvm -D LLVM_ENABLE_PROJECTS="mlir;flang;clang;llvm" -G Ninja -D 
CMAKE_BUILD_TYPE=Release -D LLVM_ENABLE_ASSERTIONS=ON -D LLVM_BUILD_EXAMPLES=ON 
-D LLVM_LIT_ARGS="-v --xunit-xml-output test-results.xml" -D LLVM_ENABLE_LLD=ON 
-D CMAKE_CXX_FLAGS=-gmlt -DBOLT_CLANG_EXE=/usr/bin/clang

You can see there 
 that 
`libBye.so` is not built. Could you try that command, pls?

In D129156#3849487 , @tarunprabhu 
wrote:

> I imagine that we want the feature to be tested even when building flang 
> out-of-tree.

I don't built standalone LLVM Flang, so won't be needing it. Perhaps somebody 
else will, dunno :)


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

https://reviews.llvm.org/D129156

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129156: Add -fpass-plugin option to Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D129156#3848704 , @clementval 
wrote:

>>> I still cannot reproduce the build failure on my end. @MatsPetersson tested 
>>> this patch and the tests passed.
>>
>> @MatsPetersson & @clementval , could you share you build command so that the 
>> failure can be reproduced before this re-lands?
>
> I shared it with @tarunprabhu and I think the only major difference is the 
> use of Ninja vs. Unix Makefiles.
>
>   cmake \
> -G "Unix Makefiles" \
> -DCMAKE_BUILD_TYPE=Release \
> -DLLVM_ENABLE_ASSERTIONS=ON \
> -DLLVM_TARGETS_TO_BUILD=host \
> -DLLVM_ENABLE_PROJECTS="clang;mlir;flang" \
> -DLLVM_ENABLE_RUNTIMES="compiler-rt"

Thanks Valentin! Switching between generators will definitely change the order 
in which libraries will built (and, AFAIK, the order is non-deterministic to 
begin with). I will try to experiment later today.

If anyone manages to test this in the meantime, please share here - that's part 
of the public review process :)


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

https://reviews.llvm.org/D129156

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129156: Add -fpass-plugin option to Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D129156#3847396 , @tarunprabhu 
wrote:

> Added `examples` to `REQUIRES` in `test/Driver/pass-plugin.f90.

Thanks for the update!

> I still cannot reproduce the build failure on my end. @MatsPetersson tested 
> this patch and the tests passed.

@MatsPetersson & @clementval , could you share you build command so that the 
failure can be reproduced before this re-lands?

> Could someone else test this on their build and let me know if it works - 
> especially if the previous version failed for you.

I can try as soon as folks share their build commands. In general, I'm 
concerned that this fix is not enough. It merely makes sure that 
"pass-plugin.f90" is only run when `LLVM_BUILD_EXAMPLES` is set (or whatever 
other flag that sets `examples` in LIT). However, that's not sufficient to make 
sure that `libBye.so` is built when running `ninja check-flang`. I might wrong 
though - have you checked this?


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

https://reviews.llvm.org/D129156

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129156: Add -fpass-plugin option to Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

I suspect that this fails when running `ninja check-flang`, right?

Most likely `Bye` needs to be added as a dependency for Flang tests, something 
akin to this 
.
  Alternatively, try adding `examples` to `! REQUIRES:` in pass-plugin.f90.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129156

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D134821: [flang][driver] Allow main program to be in an archive

[Andrzej Warzynski via Phabricator via cfe-commits]

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

Great stuff @ekieri , thanks for doing this!

You may want add a note in the summary that you've updated the docs as well.  
This is consistent with what we discussed in 
https://github.com/llvm/llvm-project/issues/54787. I've left a few minor 
comments, but otherwise LGTM.




Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:604-606
+// A Fortran main program will be lowered to a function named _QQmain. Make
+// _QQmain an undefined symbol, so that we include it even if it hides
+// inside an archive.

I would add a reference to the GitHub issue too.



Comment at: flang/docs/FlangDriver.md:152-158
-Note that currently Flang does not support code-generation and `flang-new` will
-fail during the second step above with the following error:
-```bash
-error: code-generation is not available yet
-```
-The other phases are printed nonetheless when using `-ccc-print-phases`, as
-that reflects what `clangDriver`, the library, will try to create and run.

Oh, good catch, completely forgot about this!

This is an unrelated change, but the noise is low and I'm happy for it to be 
included. You can also submit it as a separate patch if you want.



Comment at: flang/docs/FlangDriver.md:202
+## Linker invocation
+Note: Linker invocation through the flang-new driver is so far
+experimental. This section describes the currently intended design, and not

awarzynski wrote:
> I used similar syntax in 
> https://github.com/llvm/llvm-project/blob/release/14.x/flang/docs/FlangDriver.md.
>  WDYT?




Comment at: flang/docs/FlangDriver.md:202-204
+Note: Linker invocation through the flang-new driver is so far
+experimental. This section describes the currently intended design, and not
+necessarily what is implemented.

I used similar syntax in 
https://github.com/llvm/llvm-project/blob/release/14.x/flang/docs/FlangDriver.md.
 WDYT?



Comment at: flang/docs/FlangDriver.md:208
+```bash
+flang-new prog.f90
+```

;-)



Comment at: flang/docs/FlangDriver.md:211
+will, on a high level, do two things:
+* call the frontend driver to build the object file prog.o, and
+* call the system linker to build the executable a.out.





Comment at: flang/docs/FlangDriver.md:214
+
+In both invocations, flang-new will add default options. To see the exact
+invocations on your system, you can call

> add default options

Perhaps clarify "where" (i.e. to the frontend driver and the linker 
invocations).



Comment at: flang/docs/FlangDriver.md:220
+The link line will contain a fair number of options, which will depend on your
+system. Compared to when linking a c program with clang, the main additions are
+(on GNU/linux),





Comment at: flang/docs/FlangDriver.md:223
+* `--undefined=_QQmain`: A fortran main program will appear in the 
corresponding
+  object file as a function called `_QQmain`. This flag lets an object file
+  containing a fortran main program (i.e., a symbol `_QQmain`) be included in





Comment at: flang/docs/FlangDriver.md:224
+  object file as a function called `_QQmain`. This flag lets an object file
+  containing a fortran main program (i.e., a symbol `_QQmain`) be included in
+  the linking also when it is bundled in an archive.





Comment at: flang/docs/FlangDriver.md:226
+  the linking also when it is bundled in an archive.
+* `-lFortran_main`: The Fortran_main archive is part of flang's runtime. It
+  exports the symbol `main`, i.e., a c main function, which will make some





Comment at: flang/docs/FlangDriver.md:229
+  initial configuration before calling `_QQmain`, and clean up before 
returning.
+* `-lFortranRuntime`: Flang's fortran runtime, containing, for example,
+  implementations of intrinsic functions.





Comment at: flang/docs/FlangDriver.md:231
+  implementations of intrinsic functions.
+* `-lFortranDecimal`: Part of flang's runtime, containing routines for parsing
+  and formatting decimal numbers.





Comment at: flang/docs/FlangDriver.md:233
+  and formatting decimal numbers.
+* `-lm`: Link with the math library, on which flang's runtime depends.
+





Comment at: flang/test/Driver/link-c-main.c:7
+
+REQUIRES: x86-registered-target, system-linux, c-compiler
+

Same for the other test. Let me know if you'd like somebody to test this for 
you on AArch64 ;-)



Comment at: flang/test/Driver/link-c-main.c:25-26
+/*
+CHECK-DAG: F .text {{[a-f0-9]+}} main
+CHECK-DAG: *UND* {{[a-f0-9]+}} 

[PATCH] D132513: [lit] Implement DEFINE and REDEFINE directives

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

I've skimmed through - these fixes make sense to me. Thank you for the quick 
summary!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: 
flang/include/flang/Optimizer/Builder/Runtime/EnvironmentDefaults.h:7
+//
+//===--===//
+

Could you document what these are? And what are they used for?



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:403-413
+  if (const auto *arg =
+  args.getLastArg(clang::driver::options::OPT_fconvert_EQ)) {
+auto parseConvertArg = [](const char *s) {
+  return llvm::StringSwitch>(s)
+  .Case("unknown", "UNKNOWN")
+  .Case("native", "NATIVE")
+  .Case("little-endian", "LITTLE_ENDIAN")

I'm OK with a lambda here, just pointing our that in other cases we added small 
hooks, e.g. `getOptimizationLevel`. I personally prefer hooks as this means 
that methods like `parseFrontendArgs` can be a bit shorter. But this is a very 
weak preference!



Comment at: flang/test/Driver/convert.f90:1
+! Ensure argument -fconvert= works as expected.
+

Could you be more specific? IIUC, this is more about making sure that the 
option parser works correctly and reports invalid usage of `-fconvert` as an 
error, right?



Comment at: flang/test/Driver/emit-mlir.f90:16
 ! CHECK-NEXT: }
+! CHECK-NEXT: fir.global @_QQEnvironmentDefaults constant : 
!fir.ref, 
!fir.ref> {
+! CHECK-NEXT:  %[[VAL_0:.*]] = fir.zero_bits !fir.ref, !fir.ref>

peixin wrote:
> jpenix-quic wrote:
> > peixin wrote:
> > > jpenix-quic wrote:
> > > > peixin wrote:
> > > > > jpenix-quic wrote:
> > > > > > peixin wrote:
> > > > > > > Is it possible not to generated this global variable if 
> > > > > > > `fconvert=` is not specified?
> > > > > > I'm not entirely sure--the issue I was running into was how to 
> > > > > > handle this in Fortran_main.c in a way which worked for all of 
> > > > > > GCC/Clang/Visual Studio (and maybe others?). I was originally 
> > > > > > thinking of doing this by using a weak definition of 
> > > > > > _QQEnvironmentDefaults set to nullptr so fconvert, etc. could 
> > > > > > override this definition without explicitly generating the fallback 
> > > > > > case. For GCC/clang, I think I could use __attribute__((weak)), but 
> > > > > > I wasn't sure how to handle this if someone tried to build with 
> > > > > > Visual Studio (or maybe another toolchain). I saw a few workarounds 
> > > > > > (ex: 
> > > > > > https://devblogs.microsoft.com/oldnewthing/20200731-00/?p=104024) 
> > > > > > but I shied away from this since it seems to be an undocumented 
> > > > > > feature (and presumably only helps with Visual Studio). 
> > > > > > 
> > > > > > Do you know of a better or more general way I could do this? (Or, 
> > > > > > is there non-weak symbol approach that might be better that I'm 
> > > > > > missing?)
> > > > > How about generate one runtime function with the argument of 
> > > > > `EnvironmentDefaultList`? This will avoid this and using one extern 
> > > > > variable?
> > > > > 
> > > > > If users use one variable with bind C name `_QQEnvironmentDefaults` 
> > > > > in fortran or one variable with name `_QQEnvironmentDefaults` in C, 
> > > > > it is risky. Would using the runtime function and static variable 
> > > > > with the type `EnvironmentDefaultList` in runtime be safer?
> > > > Agreed that there are potential risks with the current approach 
> > > > (although, are the `_Q*` names considered reserved?). Unfortunately, I 
> > > > think generating a call to set the environment defaults requires 
> > > > somewhat significant changes to the runtime. The runtime reads 
> > > > environment variables during initialization in 
> > > > `ExecutionEnvironment::Configure` which is ultimately called from the 
> > > > "hardcoded" `Fortran_main.c` and I need to set the defaults before this 
> > > > happens. So, I believe I'd either have to move the initialization to 
> > > > `_QQmain`  or make it so that `main` isn't hardcoded so that I could 
> > > > insert the appropriate runtime function.
> > > > 
> > > > @klausler I think I asked you about this when I was first trying to 
> > > > figure out how to implement the environment defaults and you suggested 
> > > > I try the extern approach--please let me know if you have 
> > > > thoughts/suggestions around this!
> > > This is what @klausler suggested:
> > > ```
> > > Instead of adding new custom APIs that let command-line options control 
> > > behavior in a way that is redundant with the runtime environment, I 
> > > suggest that you try a more general runtime library API by which the main 
> > > program can specify a default environment variable setting, or a set of 
> > > them. Then turn the command-line options into the equivalent environment 
> > > settings and pass them as default settings that could be overridden by 
> > > the actual environment.
> > > ```
> > > If I understand correctly, what I 

[PATCH] D131475: [Flang] Use find_program() to find clang-tblgen

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for the quick fix, makes sense! 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131475

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131533: [Flang][Driver] Enable PIC in the frontend

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski accepted this revision.
awarzynski added a comment.

Thanks for all the updates and for working on this! I'm not an expert in the 
semantics of `-fpie`/`-fpic`/`-mrelocation-model`, but this basically 
replicates the logic in Clang and I am not aware of any good reasons for Flang 
to diverge from that. This looks good to me.

I've left a few minor comments inline and would appreciate if you could address 
them before landing this. Thanks again for taking this on!




Comment at: clang/include/clang/Driver/Options.td:6320-6325
+def pic_level : Separate<["-"], "pic-level">,
+  HelpText<"Value for __PIC__">,
+  MarshallingInfoInt>;
+def pic_is_pie : Flag<["-"], "pic-is-pie">,
+  HelpText<"File is for a position independent executable">,
+  MarshallingInfoFlag>;

MaskRay wrote:
> awarzynski wrote:
> > awarzynski wrote:
> > > These are code-gen options to me. While originally located under 
> > > "Language Options", I think that it would make more sense to move them 
> > > near "CodeGen Options" instead (e.g. near `mrelocation_model`). @MaskRay 
> > > any thoughts?
> > Turns out that in Clang these options are indeed [[ 
> > https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/LangOptions.def#L199
> >  | LangOptions ]]. That's a bit confusing to me, but oh well.
> clang/lib/Frontend/InitPreprocessor.cpp defines `__PIC__`.. IIUC the file 
> does not know CodeGenOptions, so LangOptions isn't a bad choice.
I think that we all agree that these options should remain somewhere withing 
the "Language Options" block, i.e. below:

```
//===--===//
// Language Options
//===--===//
```

As previously, you can use a `let` statement there:
```
let Flags = [CC1Option, FC1Option, NoDriverOption] in {
def pic_level : Separate<["-"], "pic-level">,
  HelpText<"Value for __PIC__">,
  MarshallingInfoInt>;
def pic_is_pie : Flag<["-"], "pic-is-pie">,
  HelpText<"File is for a position independent executable">,
  MarshallingInfoFlag>;

} // let Flags = [CC1Option, FC1Option, NoDriverOption]
```



Comment at: clang/include/clang/Driver/Options.td:5245
+
+} // let Flags = [CC1Option, CC1AsOption, NoDriverOption]
+





Comment at: flang/test/Driver/pic-flags.f90:1
-! Verify that in contrast to Clang, Flang does not default to generating 
position independent executables/code
 




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

https://reviews.llvm.org/D131533

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131808: [clang,flang] Add missing options fsyntax-only in help

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski accepted this revision.
awarzynski added a comment.

LGTM, thanks for the fix!

You may want to update to title to better reflect the contents (e.g. “Add help 
text for -fsyntax-only”). While the fact that it fixes 
https://github.com/llvm/llvm-project/issues/57033 motivated this change, the 
actual implementation does a bit more than just fixing that.

I can land this for you once I can access stable internet. Sorry for the delay.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131808

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131808: [clang,flang] Add missing options fsyntax-only in help

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks, mostly makes sense! https://github.com/llvm/llvm-project/issues/57033 
mentions `-O{n}` as well :) Could you fix that too? More comments inline.

CI is still failing :( Are you able to re-produce that? (I'm traveling atm, so 
can't check).




Comment at: clang/include/clang/Driver/Options.td:2798-2799
 def fsyntax_only : Flag<["-"], "fsyntax-only">,
-  Flags<[NoXarchOption,CoreOption,CC1Option,FC1Option]>, Group;
+  Flags<[NoXarchOption,CoreOption,CC1Option,FC1Option,FlangOption]>, 
Group,
+  HelpText<"Syntax-check only">;
 def ftabstop_EQ : Joined<["-"], "ftabstop=">, Group;

Note that:

```
$ gcc --help=common | grep syntax
  -fsyntax-only   Check for syntax errors, then stop.
```

and (from Clang [[ 
https://github.com/llvm/llvm-project/blob/main/clang/docs/CommandGuide/clang.rst
 | docs ]]
>  Run the preprocessor, parser and type checking stages.

For consistency sake, I would replace "Syntax-check only"" with "Run the 
preprocessor, parser and type checking stages.". Technically, there's no 
separate preprocessor in Flang, but that's IMO tangential to this.



Comment at: clang/include/clang/Driver/Options.td:3213-3214
 def headerpad__max__install__names : Joined<["-"], 
"headerpad_max_install_names">;
-def help : Flag<["-", "--"], "help">, Flags<[CC1Option,CC1AsOption, FC1Option,
-FlangOption]>, HelpText<"Display available options">,
+def help : Flag<["-", "--"], "help">, 
Flags<[CC1Option,CC1AsOption,FC1Option,FlangOption]>,
+  HelpText<"Display available options">,
 MarshallingInfoFlag>;

Unrelated change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131808

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131808: [clang,flang] Add missing options fsyntax-only in help

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

This is primarily a Clang change, so added some Clang reviewers. I will review 
shortly - thanks for taking this on!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131808

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131533: [Flang][Driver] Enable PIC in the frontend

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Left a few more comments and also added Diana as a reviewer. Would be good to 
get an extra pair of eyes on this :)




Comment at: clang/include/clang/Driver/Options.td:6320-6325
+def pic_level : Separate<["-"], "pic-level">,
+  HelpText<"Value for __PIC__">,
+  MarshallingInfoInt>;
+def pic_is_pie : Flag<["-"], "pic-is-pie">,
+  HelpText<"File is for a position independent executable">,
+  MarshallingInfoFlag>;

awarzynski wrote:
> These are code-gen options to me. While originally located under "Language 
> Options", I think that it would make more sense to move them near "CodeGen 
> Options" instead (e.g. near `mrelocation_model`). @MaskRay any thoughts?
Turns out that in Clang these options are indeed [[ 
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/LangOptions.def#L199
 | LangOptions ]]. That's a bit confusing to me, but oh well.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:120
 
+  // -fPIC/-fPIE and their variants. Similar to clang.
+  llvm::Reloc::Model RelocationModel;

awarzynski wrote:
> I would skip "Similar to clang" - this applies most of things here.
Also, I'd move this whole block to a dedicated method.



Comment at: flang/include/flang/Frontend/FrontendOptions.h:290-295
+  /// Some targets need the pic levels. These are stored as
+  /// LLVM module flags.
+  llvm::Optional PICLevel;
+  llvm::Optional PIELevel;
+  /// For setting up the target machine.
+  llvm::Optional RelocModel;

To me these are CodeGen options. Could you move this to 
https://github.com/llvm/llvm-project/blob/main/flang/include/flang/Frontend/CodeGenOptions.h?



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:151-162
+  if (model.equals("static"))
+return llvm::Reloc::Static;
+  if (model.equals("pic"))
+return llvm::Reloc::PIC_;
+  if (model.equals("dynamic-no-pic"))
+return llvm::Reloc::DynamicNoPIC;
+  if (model.equals("ropi"))

Only `-fpic` and `-fpie` are tested/supported right? Please, could you trim 
this accordingly? Or, alternatively, expand the test.



Comment at: flang/test/Driver/pic-flags.f90:3
 
-! RUN: %flang -### %s --target=aarch64-linux-gnu 2>&1 | FileCheck %s 
--check-prefix=CHECK-NOPIE
-! RUN: %flang -### %s --target=aarch64-linux-gnu -fno-pie 2>&1 | FileCheck %s 
--check-prefix=CHECK-NOPIE
-
-! RUN: %flang -### %s --target=aarch64-linux-gnu -fpie 2>&1 | FileCheck %s 
--check-prefix=CHECK-PIE
-
-! CHECK-NOPIE: "-fc1"
-! CHECk-NOPIE-NOT: "-fpic"
-! CHECK-NOPIE: "{{.*}}ld"
-! CHECK-NOPIE-NOT: "-pie"
-
-! CHECK-PIE: "-fc1"
-!! TODO Once Flang supports `-fpie`, it //should// use -fpic when invoking 
`flang -fc1`. Update the following line once `-fpie` is
-! available.
-! CHECk-PIE-NOT: "-fpic"
-! CHECK-PIE: "{{.*}}ld"
-! CHECK-PIE-NOT: "-pie"
+! RUN: %flang -v -S -emit-llvm -o - %s --target=aarch64-linux-gnu -fno-pie 
2>&1 | FileCheck %s --check-prefix=CHECK-NOPIE
+

mnadeem wrote:
> awarzynski wrote:
> > Why would you replace `-###` with `-v`? Same for other RUN lines. 
> I needed the command to run because I added check lines for the emitted LLVM 
> IR, for example:
> ! CHECK-PIE-LEVEL1: !"PIC Level", i32 1}
> ! CHECK-PIE-LEVEL1: !"PIE Level", i32 1}
Ah, of course! Thanks, I missed that earlier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131533

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131533: [Flang][Driver] Enable PIC in the frontend

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Many thanks for implementing this! I've only skimmed through so far, but mostly 
makes sense. Will take a closer look later.

Could you update the summary by adding more detail? What options are enabled 
and whether the semantics from Clang are preserved or not (they should unless 
there is a good reason not to)? It would help if you could list all of them. 
More comments inline.




Comment at: clang/include/clang/Driver/Options.td:5954-5959
+def mrelocation_model : Separate<["-"], "mrelocation-model">,
+  HelpText<"The relocation model to use">, 
Values<"static,pic,ropi,rwpi,ropi-rwpi,dynamic-no-pic">,
+  Flags<[CC1Option, CC1AsOption, FC1Option, NoDriverOption]>,
+  NormalizedValuesScope<"llvm::Reloc">,
+  NormalizedValues<["Static", "PIC_", "ROPI", "RWPI", "ROPI_RWPI", 
"DynamicNoPIC"]>,
+  MarshallingInfoEnum, "PIC_">;

As per comments in Options.td, this is a "Code-gen Option" rather than a 
"Language Option". Could you move it back somewhere near the original [[ 
https://github.com/llvm/llvm-project/blob/e5e93b6130bde96d7e14851e218c5bf055f8a834/clang/include/clang/Driver/Options.td#L5231-L5233
 | comment ]]?

I would also suggest using `let` (there's going to be more options like this):
```
let Flags = [CC1Option, CC1AsOption, FC1Option, NoDriverOption] {

def mrelocation_model : Separate<["-"], "mrelocation-model">,
  HelpText<"The relocation model to use">, 
Values<"static,pic,ropi,rwpi,ropi-rwpi,dynamic-no-pic">,
  NormalizedValuesScope<"llvm::Reloc">,
  NormalizedValues<["Static", "PIC_", "ROPI", "RWPI", "ROPI_RWPI", 
"DynamicNoPIC"]>,
  MarshallingInfoEnum, "PIC_">;

} // let Flags = [CC1Option, CC1AsOption, FC1Option, NoDriverOption]
```



Comment at: clang/include/clang/Driver/Options.td:6320-6325
+def pic_level : Separate<["-"], "pic-level">,
+  HelpText<"Value for __PIC__">,
+  MarshallingInfoInt>;
+def pic_is_pie : Flag<["-"], "pic-is-pie">,
+  HelpText<"File is for a position independent executable">,
+  MarshallingInfoFlag>;

These are code-gen options to me. While originally located under "Language 
Options", I think that it would make more sense to move them near "CodeGen 
Options" instead (e.g. near `mrelocation_model`). @MaskRay any thoughts?



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:120
 
+  // -fPIC/-fPIE and their variants. Similar to clang.
+  llvm::Reloc::Model RelocationModel;

I would skip "Similar to clang" - this applies most of things here.



Comment at: flang/test/Driver/pic-flags.f90:1
 ! Verify that in contrast to Clang, Flang does not default to generating 
position independent executables/code
 

This comment is no longer valid



Comment at: flang/test/Driver/pic-flags.f90:3
 
-! RUN: %flang -### %s --target=aarch64-linux-gnu 2>&1 | FileCheck %s 
--check-prefix=CHECK-NOPIE
-! RUN: %flang -### %s --target=aarch64-linux-gnu -fno-pie 2>&1 | FileCheck %s 
--check-prefix=CHECK-NOPIE
-
-! RUN: %flang -### %s --target=aarch64-linux-gnu -fpie 2>&1 | FileCheck %s 
--check-prefix=CHECK-PIE
-
-! CHECK-NOPIE: "-fc1"
-! CHECk-NOPIE-NOT: "-fpic"
-! CHECK-NOPIE: "{{.*}}ld"
-! CHECK-NOPIE-NOT: "-pie"
-
-! CHECK-PIE: "-fc1"
-!! TODO Once Flang supports `-fpie`, it //should// use -fpic when invoking 
`flang -fc1`. Update the following line once `-fpie` is
-! available.
-! CHECk-PIE-NOT: "-fpic"
-! CHECK-PIE: "{{.*}}ld"
-! CHECK-PIE-NOT: "-pie"
+! RUN: %flang -v -S -emit-llvm -o - %s --target=aarch64-linux-gnu -fno-pie 
2>&1 | FileCheck %s --check-prefix=CHECK-NOPIE
+

Why would you replace `-###` with `-v`? Same for other RUN lines. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131533

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for working on this. This is not my area of expertise, so I focused on 
the implementation in the driver.




Comment at: clang/include/clang/Driver/Options.td:4897
 def ffixed_line_length_VALUE : Joined<["-"], "ffixed-line-length-">, 
Group, Alias;
+def fconvert_EQ : Joined<["-"], "fconvert=">, Group,
+  HelpText<"Set endian conversion of data for unformatted files">;

jpenix-quic wrote:
> peixin wrote:
> > Why do you move it here? Maybe it is not implemented now, clang may need 
> > this option eventually. @MaskRay 
> I was using the fixed line length options as a reference for how to handle 
> this--based on the discussion in the review here 
> (https://reviews.llvm.org/D95460) about forwarding options to gfortran, I was 
> thinking that it would also be safe to handle fconvert similarly, but I'm not 
> 100% sure and definitely might be misunderstanding something!
GFortran support has been effectively broken since LLVM 12 (see e.g. this [[ 
https://github.com/llvm/llvm-project/commit/6a75496836ea14bcfd2f4b59d35a1cad4ac58cee
 | change ]]). We would do ourselves a favor if we just removed it altogether 
(I'm not aware of anyone requiring  it). 

And if Clang ever needs this option, we can always update this definition 
accordingly. No need to optimize for hypothetical scenarios that may or may not 
happen :) To me, this change makes perfect sense.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:52
+options::OPT_fno_automatic,
+options::OPT_fconvert_EQ});
 }

jpenix-quic wrote:
> Reading through https://reviews.llvm.org/D95460 again, I'm not sure this is 
> the appropriate place to add . I am marking this as a TODO that I will 
> revisit with the other feedback!
You can use `AddOtherOptions` instead. `AddFortranDialectOptions` is more about 
language options. Is this a language option? It's a bit of a borderline. Feel 
free to add another hook too.

Btw, the reformatting is an unrelated change. Could you submit in a separate 
patch? Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130078: [flang][nfc] Rename `AddOtherOptions` as `ForwardOptions`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski updated this revision to Diff 447250.
awarzynski added a comment.

Change from CamelCase to camelCase in Flang.h so that the function names adhere 
to LLVM's coding style.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130078

Files:
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/ToolChains/Flang.h


Index: clang/lib/Driver/ToolChains/Flang.h
===
--- clang/lib/Driver/ToolChains/Flang.h
+++ clang/lib/Driver/ToolChains/Flang.h
@@ -29,7 +29,7 @@
   ///
   /// \param [in] Args The list of input driver arguments
   /// \param [out] CmdArgs The list of output command arguments
-  void AddFortranDialectOptions(const llvm::opt::ArgList ,
+  void addFortranDialectOptions(const llvm::opt::ArgList ,
 llvm::opt::ArgStringList ) const;
 
   /// Extract preprocessing options from the driver arguments and add them to
@@ -37,15 +37,15 @@
   ///
   /// \param [in] Args The list of input driver arguments
   /// \param [out] CmdArgs The list of output command arguments
-  void AddPreprocessingOptions(const llvm::opt::ArgList ,
+  void addPreprocessingOptions(const llvm::opt::ArgList ,
llvm::opt::ArgStringList ) const;
-  /// Extract other compilation options from the driver arguments and add them
-  /// to the command arguments.
+
+  /// This method will effectively copy options from \a Args into \a CmdArgs.
   ///
   /// \param [in] Args The list of input driver arguments
   /// \param [out] CmdArgs The list of output command arguments
-  void AddOtherOptions(const llvm::opt::ArgList ,
-   llvm::opt::ArgStringList ) const;
+  void forwardOptions(const llvm::opt::ArgList ,
+  llvm::opt::ArgStringList ) const;
 
 public:
   Flang(const ToolChain );
Index: clang/lib/Driver/ToolChains/Flang.cpp
===
--- clang/lib/Driver/ToolChains/Flang.cpp
+++ clang/lib/Driver/ToolChains/Flang.cpp
@@ -27,7 +27,7 @@
   CmdArgs.push_back(types::getTypeName(Input.getType()));
 }
 
-void Flang::AddFortranDialectOptions(const ArgList ,
+void Flang::addFortranDialectOptions(const ArgList ,
  ArgStringList ) const {
   Args.AddAllArgs(
   CmdArgs, {options::OPT_ffixed_form, options::OPT_ffree_form,
@@ -44,14 +44,14 @@
 options::OPT_fno_automatic});
 }
 
-void Flang::AddPreprocessingOptions(const ArgList ,
+void Flang::addPreprocessingOptions(const ArgList ,
 ArgStringList ) const {
   Args.AddAllArgs(CmdArgs,
   {options::OPT_P, options::OPT_D, options::OPT_U,
options::OPT_I, options::OPT_cpp, options::OPT_nocpp});
 }
 
-void Flang::AddOtherOptions(const ArgList , ArgStringList ) const 
{
+void Flang::forwardOptions(const ArgList , ArgStringList ) const {
   Args.AddAllArgs(CmdArgs,
   {options::OPT_module_dir, options::OPT_fdebug_module_writer,
options::OPT_fintrinsic_modules_path, options::OPT_pedantic,
@@ -105,9 +105,9 @@
   // Add preprocessing options like -I, -D, etc. if we are using the
   // preprocessor (i.e. skip when dealing with e.g. binary files).
   if (types::getPreprocessedType(InputType) != types::TY_INVALID)
-AddPreprocessingOptions(Args, CmdArgs);
+addPreprocessingOptions(Args, CmdArgs);
 
-  AddFortranDialectOptions(Args, CmdArgs);
+  addFortranDialectOptions(Args, CmdArgs);
 
   // Color diagnostics are parsed by the driver directly from argv and later
   // re-parsed to construct this job; claim any possible color diagnostic here
@@ -117,8 +117,8 @@
   if (D.getDiags().getDiagnosticOptions().ShowColors)
 CmdArgs.push_back("-fcolor-diagnostics");
 
-  // Add other compile options
-  AddOtherOptions(Args, CmdArgs);
+  // Handle options which are simply forwarded to -fc1.
+  forwardOptions(Args, CmdArgs);
 
   // Forward -Xflang arguments to -fc1
   Args.AddAllArgValues(CmdArgs, options::OPT_Xflang);


Index: clang/lib/Driver/ToolChains/Flang.h
===
--- clang/lib/Driver/ToolChains/Flang.h
+++ clang/lib/Driver/ToolChains/Flang.h
@@ -29,7 +29,7 @@
   ///
   /// \param [in] Args The list of input driver arguments
   /// \param [out] CmdArgs The list of output command arguments
-  void AddFortranDialectOptions(const llvm::opt::ArgList ,
+  void addFortranDialectOptions(const llvm::opt::ArgList ,
 llvm::opt::ArgStringList ) const;
 
   /// Extract preprocessing options from the driver arguments and add them to
@@ -37,15 +37,15 @@
   ///
   /// \param [in] Args The list of input driver arguments
   /// \param [out] CmdArgs The list of output command arguments
-  void AddPreprocessingOptions(const llvm::opt::ArgList ,
+  void 

[PATCH] D130078: [flang][nfc] Rename `AddOtherOptions` as `ForwardOptions`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D130078#3673288 , @MaskRay wrote:

> In D130078#3669072 , @awarzynski 
> wrote:
>
>> In D130078#3667188 , @MaskRay 
>> wrote:
>>
>>> `forwardOptions` will be better if you are renaming it anyway.
>>
>> I'd rather create a separate patch and update all other methods to follow 
>> LLVM's style. Any idea why the style is not followed in Clang.h 
>> ?
>
> Clang is traditionally unfortunately very inconsistent in the code style 
> When you add new functions, you don't necessarily follow the Clang tradition 
> ;-)

That's a good point :) In fact, let me update other methods in this file as 
well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130078

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129864: [Flang] Generate documentation for compiler flags

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

LGTM, thanks!

(feel free to  address my [nit] when merging or ignore altogether)




Comment at: flang/docs/CMakeLists.txt:128
 
+  set(CLANG_TABLEGEN_EXE clang-tblgen)
+  gen_rst_file_from_td(FlangCommandLineReference.rst -gen-opt-docs 
../include/flang/FlangOptionsDocs.td docs-flang-html)

[nit] This is a bit out of place without a comment :) Could you mention that 
this CMake variable is required in `clang_tablegen`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129864

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130254: [CMake][Clang] Copy folder without permissions

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

What about similar code in LLVM 
?




Comment at: clang/cmake/modules/CMakeLists.txt:35
 # via CMAKE_MODULE_PATH, place API modules next to it.
+# Copy without source permissions because the source could be read-only
 file(COPY .

[nit] This comment tells me "what" is happening here, but that can be deduced 
from the code (i.e. `NO_SOURCE_PERMISSIONS` --> copy without source 
permissions). Explaining "why" this being read-only is a problem would be more 
helpful, IMHO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130254

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130078: [flang][nfc] Rename `AddOtherOptions` as `ForwardOptions`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D130078#3667188 , @MaskRay wrote:

> `forwardOptions` will be better if you are renaming it anyway.

I'd rather create a separate patch and update all other methods to follow 
LLVM's style. Any idea why the style is not followed in Clang.h 
?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130078

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129864: [Flang] Generate documentation for compiler flags

[Andrzej Warzynski via Phabricator via cfe-commits]

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

The change in ClangOptionDocEmitter.cpp is required for Flang as it heavily 
relies on these "include" flags defined in Options.td 
.
 This won't affect Clang as it does not use `IncludeFlags` in 
ClangOptionsDocs.td 
.
 If Clang devs were to use them in "ClangOptionsDocs.td", the required hooks in 
ClangOptionDocEmitter.cpp will already be there :)

LGTM, thanks for working on this! Give it another day before merging - in case 
Clang folks would like to chime in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129864

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130078: [flang][nfc] Rename `AddOtherOptions` as `ForwardOptions`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski created this revision.
awarzynski added a reviewer: tarunprabhu.
Herald added a reviewer: sscalpone.
Herald added a project: All.
awarzynski requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.

The updated name better reflects what this hook is intended for.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130078

Files:
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/ToolChains/Flang.h


Index: clang/lib/Driver/ToolChains/Flang.h
===
--- clang/lib/Driver/ToolChains/Flang.h
+++ clang/lib/Driver/ToolChains/Flang.h
@@ -39,13 +39,13 @@
   /// \param [out] CmdArgs The list of output command arguments
   void AddPreprocessingOptions(const llvm::opt::ArgList ,
llvm::opt::ArgStringList ) const;
-  /// Extract other compilation options from the driver arguments and add them
-  /// to the command arguments.
+
+  /// This method will effectively copy options from \a Args into \a CmdArgs.
   ///
   /// \param [in] Args The list of input driver arguments
   /// \param [out] CmdArgs The list of output command arguments
-  void AddOtherOptions(const llvm::opt::ArgList ,
-   llvm::opt::ArgStringList ) const;
+  void ForwardOptions(const llvm::opt::ArgList ,
+  llvm::opt::ArgStringList ) const;
 
 public:
   Flang(const ToolChain );
Index: clang/lib/Driver/ToolChains/Flang.cpp
===
--- clang/lib/Driver/ToolChains/Flang.cpp
+++ clang/lib/Driver/ToolChains/Flang.cpp
@@ -51,7 +51,7 @@
options::OPT_I, options::OPT_cpp, options::OPT_nocpp});
 }
 
-void Flang::AddOtherOptions(const ArgList , ArgStringList ) const 
{
+void Flang::ForwardOptions(const ArgList , ArgStringList ) const {
   Args.AddAllArgs(CmdArgs,
   {options::OPT_module_dir, options::OPT_fdebug_module_writer,
options::OPT_fintrinsic_modules_path, options::OPT_pedantic,
@@ -117,8 +117,8 @@
   if (D.getDiags().getDiagnosticOptions().ShowColors)
 CmdArgs.push_back("-fcolor-diagnostics");
 
-  // Add other compile options
-  AddOtherOptions(Args, CmdArgs);
+  // Handle options which are simply forwarded to -fc1.
+  ForwardOptions(Args, CmdArgs);
 
   // Forward -Xflang arguments to -fc1
   Args.AddAllArgValues(CmdArgs, options::OPT_Xflang);


Index: clang/lib/Driver/ToolChains/Flang.h
===
--- clang/lib/Driver/ToolChains/Flang.h
+++ clang/lib/Driver/ToolChains/Flang.h
@@ -39,13 +39,13 @@
   /// \param [out] CmdArgs The list of output command arguments
   void AddPreprocessingOptions(const llvm::opt::ArgList ,
llvm::opt::ArgStringList ) const;
-  /// Extract other compilation options from the driver arguments and add them
-  /// to the command arguments.
+
+  /// This method will effectively copy options from \a Args into \a CmdArgs.
   ///
   /// \param [in] Args The list of input driver arguments
   /// \param [out] CmdArgs The list of output command arguments
-  void AddOtherOptions(const llvm::opt::ArgList ,
-   llvm::opt::ArgStringList ) const;
+  void ForwardOptions(const llvm::opt::ArgList ,
+  llvm::opt::ArgStringList ) const;
 
 public:
   Flang(const ToolChain );
Index: clang/lib/Driver/ToolChains/Flang.cpp
===
--- clang/lib/Driver/ToolChains/Flang.cpp
+++ clang/lib/Driver/ToolChains/Flang.cpp
@@ -51,7 +51,7 @@
options::OPT_I, options::OPT_cpp, options::OPT_nocpp});
 }
 
-void Flang::AddOtherOptions(const ArgList , ArgStringList ) const {
+void Flang::ForwardOptions(const ArgList , ArgStringList ) const {
   Args.AddAllArgs(CmdArgs,
   {options::OPT_module_dir, options::OPT_fdebug_module_writer,
options::OPT_fintrinsic_modules_path, options::OPT_pedantic,
@@ -117,8 +117,8 @@
   if (D.getDiags().getDiagnosticOptions().ShowColors)
 CmdArgs.push_back("-fcolor-diagnostics");
 
-  // Add other compile options
-  AddOtherOptions(Args, CmdArgs);
+  // Handle options which are simply forwarded to -fc1.
+  ForwardOptions(Args, CmdArgs);
 
   // Forward -Xflang arguments to -fc1
   Args.AddAllArgValues(CmdArgs, options::OPT_Xflang);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129864: [Flang] Generate documentation for compiler flags

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Makes sense, thanks for working on this! Some minor comments below and inline.

From your summary:

> This is done by using clang tablegen

What do you mean by "clang tablegen"? Is it Clang's clang_tablegen 

 CMake function?

> from options.td

Options.td ;-)

> shared with clang

Most people associate "clang" with the executable. You may want to use "Clang" 
to avoid confusion (IIUC, you are referring to the sub-project rather than the 
executable here).

> specific flang flags

Flang :) (same rationale as above)




Comment at: clang/utils/TableGen/ClangOptionDocEmitter.cpp:329
 raw_ostream ) {
-  if (isExcluded(Option.Option, DocInfo))
+  if (isExcluded(Option.Option, DocInfo) || !isIncluded(Option.Option, 
DocInfo))
 return;

I think that the way this is written at the moment is a bit counterintuitive 
(i.e. "if excluded or not included"). This is really down to how these 
include/exclude flags are used in various other places :( (i.e. there's little 
that we can do about it).

I think that it would make more sense to follow the style from [[ 
https://github.com/llvm/llvm-project/blob/f80a4321ef1bafcd8041884bcb85d9ba24335adb/llvm/lib/Option/OptTable.cpp#L656-L659
 | OptTable.cpp ]]. In particular (pseudo code):
```
if (excluded)
  return;
if (included_not_empty && !included)
  return;
```

This way it becomes clear that `IncludedFlags` (from "FlangOptionsDocs.td") is 
only taken into account when not empty. I would also add an assert in 
`isIncluded`:
```
assert(DocInfo->getValue("IncludedFlags") && Missing includeFlags);
```

WDYT? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129864

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125788: [flang][driver] Rename `flang-new` as `flang`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski updated this revision to Diff 441643.
awarzynski added a comment.

Add a comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125788

Files:
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/test/Driver/flang/flang.f90
  clang/test/Driver/flang/flang_ucase.F90
  clang/test/Driver/flang/multiple-inputs-mixed.f90
  clang/test/Driver/flang/multiple-inputs.f90
  flang/CMakeLists.txt
  flang/docs/FlangDriver.md
  flang/docs/ImplementingASemanticCheck.md
  flang/docs/Overview.md
  flang/examples/FlangOmpReport/FlangOmpReport.cpp
  flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
  flang/test/CMakeLists.txt
  flang/test/Driver/disable-ext-name-interop.f90
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-version.f90
  flang/test/Driver/escaped-backslash.f90
  flang/test/Driver/fdefault.f90
  flang/test/Driver/flarge-sizes.f90
  flang/test/Driver/frontend-forwarding.f90
  flang/test/Driver/intrinsic-module-path.f90
  flang/test/Driver/macro-def-undef.F90
  flang/test/Driver/missing-input.f90
  flang/test/Driver/predefined-macros-compiler-version.F90
  flang/test/Driver/std2018-wrong.f90
  flang/test/Driver/std2018.f90
  flang/test/Driver/use-module-error.f90
  flang/test/Driver/use-module.f90
  flang/test/Frontend/multiple-input-files.f90
  flang/test/Lower/Intrinsics/command_argument_count.f90
  flang/test/Lower/Intrinsics/exit.f90
  flang/test/Lower/Intrinsics/get_command_argument.f90
  flang/test/Lower/Intrinsics/get_environment_variable.f90
  flang/test/Lower/OpenACC/Todo/acc-declare.f90
  flang/test/Lower/OpenACC/Todo/acc-routine.f90
  flang/test/Lower/OpenMP/Todo/omp-declarative-allocate.f90
  flang/test/Lower/OpenMP/Todo/omp-declare-reduction.f90
  flang/test/Lower/OpenMP/Todo/omp-declare-simd.f90
  flang/test/Lower/OpenMP/Todo/omp-declare-target.f90
  flang/test/lit.cfg.py
  flang/test/lit.site.cfg.py.in
  flang/tools/f18/CMakeLists.txt
  flang/tools/flang-driver/CMakeLists.txt
  flang/tools/flang-driver/driver.cpp

Index: flang/tools/flang-driver/driver.cpp
===
--- flang/tools/flang-driver/driver.cpp
+++ flang/tools/flang-driver/driver.cpp
@@ -85,14 +85,15 @@
   llvm::InitLLVM x(argc, argv);
   llvm::SmallVector args(argv, argv + argc);
 
-  clang::driver::ParsedClangName targetandMode("flang", "--driver-mode=flang");
+  clang::driver::ParsedClangName targetandMode =
+  clang::driver::ToolChain::getTargetAndModeFromProgramName(argv[0]);
   std::string driverPath = getExecutablePath(args[0]);
 
   llvm::BumpPtrAllocator a;
   llvm::StringSaver saver(a);
   ExpandResponseFiles(saver, args);
 
-  // Check if flang-new is in the frontend mode
+  // Check if flang is in the frontend mode
   auto firstArg = std::find_if(
   args.begin() + 1, args.end(), [](const char *a) { return a != nullptr; });
   if (firstArg != args.end()) {
@@ -101,7 +102,7 @@
<< "Valid tools include '-fc1'.\n";
   return 1;
 }
-// Call flang-new frontend
+// Call flang frontend
 if (llvm::StringRef(args[1]).startswith("-fc1")) {
   return executeFC1Tool(args);
 }
Index: flang/tools/flang-driver/CMakeLists.txt
===
--- flang/tools/flang-driver/CMakeLists.txt
+++ flang/tools/flang-driver/CMakeLists.txt
@@ -10,7 +10,7 @@
   Support
 )
 
-add_flang_tool(flang-new
+add_flang_tool(flang
   driver.cpp
   fc1_main.cpp
 
@@ -23,23 +23,33 @@
   Fortran_main
 )
 
-target_link_libraries(flang-new
+target_link_libraries(flang
   PRIVATE
   flangFrontend
   flangFrontendTool
 )
 
-clang_target_link_libraries(flang-new
+clang_target_link_libraries(flang
   PRIVATE
   clangDriver
   clangBasic
 )
 
+if(WIN32 AND NOT CYGWIN)
+  # Prevent versioning if the buildhost is targeting for Win32.
+else()
+  set_target_properties(flang PROPERTIES VERSION ${FLANG_EXECUTABLE_VERSION})
+endif()
+
+if (FLANG_USE_LEGACY_NAME)
+  set_target_properties(flang PROPERTIES OUTPUT_NAME flang-new)
+endif()
+
 option(FLANG_PLUGIN_SUPPORT "Build Flang with plugin support." ON)
 
-# Enable support for plugins, which need access to symbols from flang-new
+# Enable support for plugins, which need access to symbols from flang
 if(FLANG_PLUGIN_SUPPORT)
-  export_executable_symbols_for_plugins(flang-new)
+  export_executable_symbols_for_plugins(flang)
 endif()
 
-install(TARGETS flang-new DESTINATION "${CMAKE_INSTALL_BINDIR}")
+install(TARGETS flang DESTINATION "${CMAKE_INSTALL_BINDIR}")
Index: flang/tools/f18/CMakeLists.txt
===
--- flang/tools/f18/CMakeLists.txt
+++ flang/tools/f18/CMakeLists.txt
@@ -35,9 +35,9 @@
   endif()
   add_custom_command(OUTPUT ${base}.mod
 COMMAND ${CMAKE_COMMAND} -E make_directory ${FLANG_INTRINSIC_MODULES_DIR}
-COMMAND 

[PATCH] D125788: [flang][driver] Rename `flang-new` as `flang`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: clang/lib/Driver/ToolChain.cpp:185
   {"flang", "--driver-mode=flang"},
+  {"flang-new", "--driver-mode=flang"},
   {"clang-dxc", "--driver-mode=dxc"},

richard.barton.arm wrote:
> clementval wrote:
> > This is counter intuitive. We rename flang-new but we add flang-new here. 
> > It should be configurable. 
> This is a list of all the valid prefixes of binaries that the driver 
> supports. With this change, an additional one will be supported so I don't 
> think it's an issue to have both flang and flang-new here.
> 
> The thing that confuses me is how flang-new works today without this change, 
> given that "flang" is not a suffix of "flang-new"? @awarzynski , do you know 
> why that is? Perhaps the line here is not needed at all? Or is this a bug fix 
> for flang-new that is actually unrelated to this change?
> This is counter intuitive. 

I can add a comment to clarify this.

> It should be configurable.

It is, in Flang's [[ 
https://github.com/llvm/llvm-project/blob/main/flang/tools/flang-driver/driver.cpp
 | driver.cpp ]]. Originally, the suffix was hard-coded as:
```
clang::driver::ParsedClangName targetandMode("flang", "--driver-mode=flang");
```
(i.e. the `clangDriver` library used `flang` internally despite the actual name 
being `flang-new`). This is now being replaced with (see in "driver.cpp"):
```
 clang::driver::ParsedClangName targetandMode =
  clang::driver::ToolChain::getTargetAndModeFromProgramName(argv[0]);
```

But the change in "driver.cpp" means that we can no longer make assumptions 
about the suffix and hence the update here. 

Like I mentioned earlier, we should not make this file build-time configurable. 
One possible option would be to try to update Clang's [[ 
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Config/config.h.cmake
 | config.h.cmake ]], but that's would lead to more Flang-specific changes in 
Clang's core set-up. Also, I'm not convinced it would work here. 

> @awarzynski , do you know why that is? 

Yeah, check Flang's "driver.cpp". We should've captured this earlier. The 
original set-up from ToolChain.cpp predates `flang-new`. And then in 
"driver.cpp" we just matched what was here. There was a lot of confusion around 
naming  back then and this has slipped in. 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125788

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125788: [flang][driver] Rename `flang-new` as `flang`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thank your for reviewing @clementval !

In D125788#3621585 , @clementval 
wrote:

> Shouldn't we just wait until we can make the permanent renaming so we do not 
> add unnecessary cmake option?

This was discussed in one of our calls. As we weren't able to agree on any 
specific time-frames, the CMake route was proposed instead. This was suggested 
by @sscalpone as an acceptable compromise. From what I recall, there were no 
objections to this.

`flang-new` has always been intended as a //temporary name// for the compiler 
driver. That's because `flang` was reserved for the bash script. The bash 
script has recently been renamed as `flang-to-external-fc` ( D125832 
) so we are free to repurpose that name.  As 
I tried to explain earlier, these names (`flang-new` vs `flang`) have always 
been very confusing to people and we are trying to improve and to clarify that, 
step by step. While this approach is not ideal, it gives us the flexibility to 
choose our preferred name sooner rather than later. If you feel that we should 
keep `flang-new`, you can continue using/building LLVM Flang as you have been 
so far. If you are ready to update the driver name, the CMake option enables 
that for you.

We discussed this in our call on Monday and agreed to go ahead provided that 
this change is technically sound. IIUC, this has now been confirmed:

> Overall the patch looks ok from a technical point.

As always, please correct me if I missed or misinterpreted something!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125788

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128333: [clang][flang] Disable defaulting to `-fpie` for LLVM Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb405407a4899: [clang][flang] Disable defaulting to `-fpie` 
for LLVM Flang (authored by awarzynski).

Changed prior to commit:
  https://reviews.llvm.org/D128333?vs=440874=440884#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128333

Files:
  clang/lib/Driver/ToolChains/Linux.cpp
  flang/docs/ReleaseNotes.md
  flang/test/Driver/pic-flags.f90


Index: flang/test/Driver/pic-flags.f90
===
--- /dev/null
+++ flang/test/Driver/pic-flags.f90
@@ -0,0 +1,24 @@
+! Verify that in contrast to Clang, Flang does not default to generating 
position independent executables/code
+
+!-
+! RUN COMMANDS
+!-
+! RUN: %flang -### %s --target=aarch64-linux-gnu 2>&1 | FileCheck %s 
--check-prefix=CHECK-NOPIE
+! RUN: %flang -### %s --target=aarch64-linux-gnu -fno-pie 2>&1 | FileCheck %s 
--check-prefix=CHECK-NOPIE
+
+! RUN: %flang -### %s --target=aarch64-linux-gnu -fpie 2>&1 | FileCheck %s 
--check-prefix=CHECK-PIE
+
+!
+! EXPECTED OUTPUT
+!
+! CHECK-NOPIE: "-fc1"
+! CHECk-NOPIE-NOT: "-fpic"
+! CHECK-NOPIE: "{{.*}}ld"
+! CHECK-NOPIE-NOT: "-pie"
+
+! CHECK-PIE: "-fc1"
+!! TODO Once Flang supports `-fpie`, it //should// use -fpic when invoking 
`flang -fc1`. Update the following line once `-fpie` is
+! available.
+! CHECk-PIE-NOT: "-fpic"
+! CHECK-PIE: "{{.*}}ld"
+! CHECK-PIE-NOT: "-pie"
Index: flang/docs/ReleaseNotes.md
===
--- flang/docs/ReleaseNotes.md
+++ flang/docs/ReleaseNotes.md
@@ -28,6 +28,13 @@
 
 ## Non-comprehensive list of changes in this release
 * The bash wrapper script, `flang`, is renamed as `flang-to-external-fc`.
+* In contrast to Clang, Flang will not default to using `-fpie` when linking
+  executables. This is only a temporary solution and the goal is to align with
+  Clang in the near future. First, however, the frontend driver needs to be
+  extended so that it can generate position independent code (that requires
+  adding support for e.g. `-fpic` and `-mrelocation-model` in `flang-new
+  -fc1`). Once that is available, support for the `-fpie` can officially be
+  added and the default behaviour updated.
 
 ## New Compiler Flags
 * Refined how `-f{no-}color-diagnostics` is treated to better align with Clang.
Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -699,8 +699,11 @@
 }
 
 bool Linux::isPIEDefault(const llvm::opt::ArgList ) const {
-  return CLANG_DEFAULT_PIE_ON_LINUX || getTriple().isAndroid() ||
- getTriple().isMusl() || getSanitizerArgs(Args).requiresPIE();
+  // TODO: Remove the special treatment for Flang once its frontend driver can
+  // generate position independent code.
+  return !getDriver().IsFlangMode() &&
+ (CLANG_DEFAULT_PIE_ON_LINUX || getTriple().isAndroid() ||
+  getTriple().isMusl() || getSanitizerArgs(Args).requiresPIE());
 }
 
 bool Linux::IsAArch64OutlineAtomicsDefault(const ArgList ) const {


Index: flang/test/Driver/pic-flags.f90
===
--- /dev/null
+++ flang/test/Driver/pic-flags.f90
@@ -0,0 +1,24 @@
+! Verify that in contrast to Clang, Flang does not default to generating position independent executables/code
+
+!-
+! RUN COMMANDS
+!-
+! RUN: %flang -### %s --target=aarch64-linux-gnu 2>&1 | FileCheck %s --check-prefix=CHECK-NOPIE
+! RUN: %flang -### %s --target=aarch64-linux-gnu -fno-pie 2>&1 | FileCheck %s --check-prefix=CHECK-NOPIE
+
+! RUN: %flang -### %s --target=aarch64-linux-gnu -fpie 2>&1 | FileCheck %s --check-prefix=CHECK-PIE
+
+!
+! EXPECTED OUTPUT
+!
+! CHECK-NOPIE: "-fc1"
+! CHECk-NOPIE-NOT: "-fpic"
+! CHECK-NOPIE: "{{.*}}ld"
+! CHECK-NOPIE-NOT: "-pie"
+
+! CHECK-PIE: "-fc1"
+!! TODO Once Flang supports `-fpie`, it //should// use -fpic when invoking `flang -fc1`. Update the following line once `-fpie` is
+! available.
+! CHECk-PIE-NOT: "-fpic"
+! CHECK-PIE: "{{.*}}ld"
+! CHECK-PIE-NOT: "-pie"
Index: flang/docs/ReleaseNotes.md
===
--- flang/docs/ReleaseNotes.md
+++ flang/docs/ReleaseNotes.md
@@ -28,6 +28,13 @@
 
 ## Non-comprehensive list of changes in this release
 * The bash wrapper script, `flang`, is renamed as `flang-to-external-fc`.
+* In contrast to Clang, Flang will not default to using `-fpie` when linking
+  executables. This is only a temporary solution and the goal is to align with
+  Clang in the near future. First, however, the frontend driver needs to be
+  extended so that it can generate position independent code (that requires
+  

[PATCH] D128333: [clang][flang] Disable defaulting to `-fpie` for LLVM Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski updated this revision to Diff 440874.
awarzynski added a comment.

Update the test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128333

Files:
  clang/lib/Driver/ToolChains/Linux.cpp
  flang/test/Driver/pic-flags.f90


Index: flang/test/Driver/pic-flags.f90
===
--- /dev/null
+++ flang/test/Driver/pic-flags.f90
@@ -0,0 +1,24 @@
+! Verify that in contrast to Clang, Flang does not default to generating 
position independent executables/code
+
+!-
+! RUN COMMANDS
+!-
+! RUN: %flang -### %s --target=aarch64-linux-gnu 2>&1 | FileCheck %s 
--check-prefix=CHECK-NOPIE
+! RUN: %flang -### %s --target=aarch64-linux-gnu -fno-pie 2>&1 | FileCheck %s 
--check-prefix=CHECK-NOPIE
+
+! RUN: %flang -### %s --target=aarch64-linux-gnu -fpie 2>&1 | FileCheck %s 
--check-prefix=CHECK-PIE
+
+!
+! EXPECTED OUTPUT
+!
+! CHECK-NOPIE: "-fc1"
+! CHECk-NOPIE-NOT: "-fpic"
+! CHECK-NOPIE: "{{.*}}ld"
+! CHECK-NOPIE-NOT: "-pie"
+
+! CHECK-PIE: "-fc1"
+!! TODO Once Flang supports `-fpie`, it //should// use -fpic when invoking 
`flang -fc1`. Update the following line once `-fpie` is
+! available.
+! CHECk-PIE-NOT: "-fpic"
+! CHECK-PIE: "{{.*}}ld"
+! CHECK-PIE-NOT: "-pie"
Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -699,8 +699,11 @@
 }
 
 bool Linux::isPIEDefault(const llvm::opt::ArgList ) const {
-  return CLANG_DEFAULT_PIE_ON_LINUX || getTriple().isAndroid() ||
- getTriple().isMusl() || getSanitizerArgs(Args).requiresPIE();
+  // TODO: Remove the special treatment for Flang once its frontend driver can
+  // generate position independent code.
+  return !getDriver().IsFlangMode() &&
+ (CLANG_DEFAULT_PIE_ON_LINUX || getTriple().isAndroid() ||
+  getTriple().isMusl() || getSanitizerArgs(Args).requiresPIE());
 }
 
 bool Linux::IsAArch64OutlineAtomicsDefault(const ArgList ) const {


Index: flang/test/Driver/pic-flags.f90
===
--- /dev/null
+++ flang/test/Driver/pic-flags.f90
@@ -0,0 +1,24 @@
+! Verify that in contrast to Clang, Flang does not default to generating position independent executables/code
+
+!-
+! RUN COMMANDS
+!-
+! RUN: %flang -### %s --target=aarch64-linux-gnu 2>&1 | FileCheck %s --check-prefix=CHECK-NOPIE
+! RUN: %flang -### %s --target=aarch64-linux-gnu -fno-pie 2>&1 | FileCheck %s --check-prefix=CHECK-NOPIE
+
+! RUN: %flang -### %s --target=aarch64-linux-gnu -fpie 2>&1 | FileCheck %s --check-prefix=CHECK-PIE
+
+!
+! EXPECTED OUTPUT
+!
+! CHECK-NOPIE: "-fc1"
+! CHECk-NOPIE-NOT: "-fpic"
+! CHECK-NOPIE: "{{.*}}ld"
+! CHECK-NOPIE-NOT: "-pie"
+
+! CHECK-PIE: "-fc1"
+!! TODO Once Flang supports `-fpie`, it //should// use -fpic when invoking `flang -fc1`. Update the following line once `-fpie` is
+! available.
+! CHECk-PIE-NOT: "-fpic"
+! CHECK-PIE: "{{.*}}ld"
+! CHECK-PIE-NOT: "-pie"
Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -699,8 +699,11 @@
 }
 
 bool Linux::isPIEDefault(const llvm::opt::ArgList ) const {
-  return CLANG_DEFAULT_PIE_ON_LINUX || getTriple().isAndroid() ||
- getTriple().isMusl() || getSanitizerArgs(Args).requiresPIE();
+  // TODO: Remove the special treatment for Flang once its frontend driver can
+  // generate position independent code.
+  return !getDriver().IsFlangMode() &&
+ (CLANG_DEFAULT_PIE_ON_LINUX || getTriple().isAndroid() ||
+  getTriple().isMusl() || getSanitizerArgs(Args).requiresPIE());
 }
 
 bool Linux::IsAArch64OutlineAtomicsDefault(const ArgList ) const {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128333: [clang][flang] Disable defaulting to `-fpie` for LLVM Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski updated this revision to Diff 440628.
awarzynski added a comment.

Update the test following the comments from @MaskRay

Also added a comment in Linux.cpp and renamed no-pie.f90 as pic-flags.f90 (to 
avoid FileCheck matching e.g. `! CHECK: pie` against the file name).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128333

Files:
  clang/lib/Driver/ToolChains/Linux.cpp
  flang/test/Driver/pic-flags.f90


Index: flang/test/Driver/pic-flags.f90
===
--- /dev/null
+++ flang/test/Driver/pic-flags.f90
@@ -0,0 +1,24 @@
+! Verify that in contrast to Clang, Flang does not default to generating 
position independent executables/code
+
+!-
+! RUN COMMANDS
+!-
+! RUN: %flang -### %s -target aarch64-linux-gnu 2>&1 | FileCheck %s 
--check-prefix=CHECK-NOPIE
+! RUN: %flang -### %s -target aarch64-linux-gnu -fno-pie 2>&1 | FileCheck %s 
--check-prefix=CHECK-NOPIE
+
+! RUN: %flang -### %s -target aarch64-linux-gnu -fpie 2>&1 | FileCheck %s 
--check-prefix=CHECK-PIE
+
+!
+! EXPECTED OUTPUT
+!
+! CHECK-NOPIE: "-fc1"
+! CHECk-NOPIE-NOT: fpic
+! CHECK-NOPIE: "{{.*}}ld"
+! CHECK-NOPIE-NOT: "-pie"
+
+! CHECK-PIE: "-fc1"
+! TODO: Once Flang supports `-fpie`, it //should// use -fpic when invoking 
`flang -fc1`. Update the following line once `-fpie` is
+! available.
+! CHECk-PIE-NOT: fpic
+! CHECK-PIE: "{{.*}}ld"
+! CHECK-PIE-NOT: "-pie"
Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -699,8 +699,11 @@
 }
 
 bool Linux::isPIEDefault(const llvm::opt::ArgList ) const {
-  return CLANG_DEFAULT_PIE_ON_LINUX || getTriple().isAndroid() ||
- getTriple().isMusl() || getSanitizerArgs(Args).requiresPIE();
+  // TODO: Remove the special treatment for Flang once its frontend driver can
+  // generate position independent code.
+  return !getDriver().IsFlangMode() &&
+ (CLANG_DEFAULT_PIE_ON_LINUX || getTriple().isAndroid() ||
+  getTriple().isMusl() || getSanitizerArgs(Args).requiresPIE());
 }
 
 bool Linux::IsAArch64OutlineAtomicsDefault(const ArgList ) const {


Index: flang/test/Driver/pic-flags.f90
===
--- /dev/null
+++ flang/test/Driver/pic-flags.f90
@@ -0,0 +1,24 @@
+! Verify that in contrast to Clang, Flang does not default to generating position independent executables/code
+
+!-
+! RUN COMMANDS
+!-
+! RUN: %flang -### %s -target aarch64-linux-gnu 2>&1 | FileCheck %s --check-prefix=CHECK-NOPIE
+! RUN: %flang -### %s -target aarch64-linux-gnu -fno-pie 2>&1 | FileCheck %s --check-prefix=CHECK-NOPIE
+
+! RUN: %flang -### %s -target aarch64-linux-gnu -fpie 2>&1 | FileCheck %s --check-prefix=CHECK-PIE
+
+!
+! EXPECTED OUTPUT
+!
+! CHECK-NOPIE: "-fc1"
+! CHECk-NOPIE-NOT: fpic
+! CHECK-NOPIE: "{{.*}}ld"
+! CHECK-NOPIE-NOT: "-pie"
+
+! CHECK-PIE: "-fc1"
+! TODO: Once Flang supports `-fpie`, it //should// use -fpic when invoking `flang -fc1`. Update the following line once `-fpie` is
+! available.
+! CHECk-PIE-NOT: fpic
+! CHECK-PIE: "{{.*}}ld"
+! CHECK-PIE-NOT: "-pie"
Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -699,8 +699,11 @@
 }
 
 bool Linux::isPIEDefault(const llvm::opt::ArgList ) const {
-  return CLANG_DEFAULT_PIE_ON_LINUX || getTriple().isAndroid() ||
- getTriple().isMusl() || getSanitizerArgs(Args).requiresPIE();
+  // TODO: Remove the special treatment for Flang once its frontend driver can
+  // generate position independent code.
+  return !getDriver().IsFlangMode() &&
+ (CLANG_DEFAULT_PIE_ON_LINUX || getTriple().isAndroid() ||
+  getTriple().isMusl() || getSanitizerArgs(Args).requiresPIE());
 }
 
 bool Linux::IsAArch64OutlineAtomicsDefault(const ArgList ) const {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128333: [clang][flang] Disable defaulting to `-fpie` for LLVM Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D128333#3613696 , @MaskRay wrote:

> True. If it is difficult to override the -pie default from flang side, I am 
> fine with the code change.

Thanks! The proper/long-term fix will require extending Flang's frontend driver 
so that it supports `-mrelocation-model`, `-fpic` and `-fpic-level` (and 
probably a few more). But I don't want to rush that.

>>> (Why does flang use clang/lib/Driver? For clang developers, it seems that 
>>> `check-clang check-clang-tools` is not sufficient. `check-flang` needs to 
>>> be used as well.)
>>
>> Otherwise, we'd have to re-implement clang/lib/Driver for LLVM Flang. This 
>> design was proposed and discussed here 
>> .
>
> OK, so you did bring this up. I guess I'll have to accept...

With clangDriver 

 being effectively shared between Clang and Flang, it would make a lot of sense 
to lift `clangDriver` out of Clang so that it's an independent sub-project. 
We've proposed that and folks have been mostly in favor. Relevant RFCs:

- RFC: refactoring libclangDriver/libclangFrontend to share with Flang 

- RFC: refactoring clangDriver - diagnostics classes 


Not much is really happening in this area ATM, but I'm hoping that 
https://discourse.llvm.org/t/rfc-improving-clang-s-diagnostics could unblock 
some progress.




Comment at: flang/test/Driver/no-pie.f90:3
+
+!-
+! RUN COMMANDS

MaskRay wrote:
> The `! RUN COMMANDS` and `EXPECTED OUTPUT` comments seem rather redundant. 
> I'd remove them.
This style is quite common in this directory, see e.g. 
https://github.com/llvm/llvm-project/blob/main/flang/test/Driver/emit-asm-aarch64.f90.
 I'm hoping that this makes these tests easier to follow for folks less 
familiar with LIT in general.

If that's OK, I'll leave this here (but I don't expect others to follow similar 
approach, IMO it's a matter of personal preference).



Comment at: flang/test/Driver/no-pie.f90:11
+!
+! CHECK-NOT: fpie
+! CHECK-NOT: fpic

MaskRay wrote:
> This NOT pattern can easily get stale. You need a test that driver `-pie` 
> forwards `-pie` to the linker and by default `-pie` does not pass the the 
> linker.
Good point, will update!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128333

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128333: [clang][flang] Disable defaulting to `-fpie` for LLVM Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

@MaskRay, thank for taking a look!

In D128333#3605745 , @MaskRay wrote:

> gfortran defaults to PIE as well.

While we strive to be compatible with `gfortan`, there's a lot relatively 
"basic" things still missing in LLVM Flang. So this is not the highest priority 
ATM.

>> We can revisit this once support for -fpie and -fpic is available in LLVM 
>> Flang. I'm not aware of anyone actively working in this area.
>
> Disagree. The PIE default should be fine. When PIE support is added, the 
> default mode naturally becomes PIE.
> Note: -fpie object files can be linked with either -no-pie or -pie. -fno-pic 
> object files can only be linked with -no-pie.
> -fpie is more portable than -fno-pic.

But an object file with R_X86_64_32 relocations cannot be linked with a `-pie` 
object, right? How can I make sure that there are no such relocations then?

> (Why does flang use clang/lib/Driver? For clang developers, it seems that 
> `check-clang check-clang-tools` is not sufficient. `check-flang` needs to be 
> used as well.)

Otherwise, we'd have to re-implement clang/lib/Driver for LLVM Flang. This 
design was proposed and discussed here 
.

> For clang developers, it seems that `check-clang check-clang-tools` is not 
> sufficient. `check-flang` needs to be used as well.)

Isn't this a bit similar to e.g. LLVM developers? Or MLIR developers?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128333

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125788: [flang][driver] Rename `flang-new` as `flang`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski updated this revision to Diff 440283.
awarzynski added a comment.

Incorporate @clementval 's suggestions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125788

Files:
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/test/Driver/flang/flang.f90
  clang/test/Driver/flang/flang_ucase.F90
  clang/test/Driver/flang/multiple-inputs-mixed.f90
  clang/test/Driver/flang/multiple-inputs.f90
  flang/CMakeLists.txt
  flang/docs/FlangDriver.md
  flang/docs/ImplementingASemanticCheck.md
  flang/docs/Overview.md
  flang/examples/FlangOmpReport/FlangOmpReport.cpp
  flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
  flang/test/CMakeLists.txt
  flang/test/Driver/disable-ext-name-interop.f90
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-version.f90
  flang/test/Driver/escaped-backslash.f90
  flang/test/Driver/fdefault.f90
  flang/test/Driver/flarge-sizes.f90
  flang/test/Driver/frontend-forwarding.f90
  flang/test/Driver/intrinsic-module-path.f90
  flang/test/Driver/macro-def-undef.F90
  flang/test/Driver/missing-input.f90
  flang/test/Driver/predefined-macros-compiler-version.F90
  flang/test/Driver/std2018-wrong.f90
  flang/test/Driver/std2018.f90
  flang/test/Driver/use-module-error.f90
  flang/test/Driver/use-module.f90
  flang/test/Frontend/multiple-input-files.f90
  flang/test/Lower/Intrinsics/command_argument_count.f90
  flang/test/Lower/Intrinsics/exit.f90
  flang/test/Lower/Intrinsics/get_command_argument.f90
  flang/test/Lower/Intrinsics/get_environment_variable.f90
  flang/test/Lower/OpenACC/Todo/acc-declare.f90
  flang/test/Lower/OpenACC/Todo/acc-routine.f90
  flang/test/Lower/OpenMP/Todo/omp-declarative-allocate.f90
  flang/test/Lower/OpenMP/Todo/omp-declare-reduction.f90
  flang/test/Lower/OpenMP/Todo/omp-declare-simd.f90
  flang/test/Lower/OpenMP/Todo/omp-declare-target.f90
  flang/test/lit.cfg.py
  flang/test/lit.site.cfg.py.in
  flang/tools/f18/CMakeLists.txt
  flang/tools/flang-driver/CMakeLists.txt
  flang/tools/flang-driver/driver.cpp

Index: flang/tools/flang-driver/driver.cpp
===
--- flang/tools/flang-driver/driver.cpp
+++ flang/tools/flang-driver/driver.cpp
@@ -85,14 +85,15 @@
   llvm::InitLLVM x(argc, argv);
   llvm::SmallVector args(argv, argv + argc);
 
-  clang::driver::ParsedClangName targetandMode("flang", "--driver-mode=flang");
+  clang::driver::ParsedClangName targetandMode =
+  clang::driver::ToolChain::getTargetAndModeFromProgramName(argv[0]);
   std::string driverPath = getExecutablePath(args[0]);
 
   llvm::BumpPtrAllocator a;
   llvm::StringSaver saver(a);
   ExpandResponseFiles(saver, args);
 
-  // Check if flang-new is in the frontend mode
+  // Check if flang is in the frontend mode
   auto firstArg = std::find_if(
   args.begin() + 1, args.end(), [](const char *a) { return a != nullptr; });
   if (firstArg != args.end()) {
@@ -101,7 +102,7 @@
<< "Valid tools include '-fc1'.\n";
   return 1;
 }
-// Call flang-new frontend
+// Call flang frontend
 if (llvm::StringRef(args[1]).startswith("-fc1")) {
   return executeFC1Tool(args);
 }
Index: flang/tools/flang-driver/CMakeLists.txt
===
--- flang/tools/flang-driver/CMakeLists.txt
+++ flang/tools/flang-driver/CMakeLists.txt
@@ -10,7 +10,7 @@
   Support
 )
 
-add_flang_tool(flang-new
+add_flang_tool(flang
   driver.cpp
   fc1_main.cpp
 
@@ -23,23 +23,33 @@
   Fortran_main
 )
 
-target_link_libraries(flang-new
+target_link_libraries(flang
   PRIVATE
   flangFrontend
   flangFrontendTool
 )
 
-clang_target_link_libraries(flang-new
+clang_target_link_libraries(flang
   PRIVATE
   clangDriver
   clangBasic
 )
 
+if(WIN32 AND NOT CYGWIN)
+  # Prevent versioning if the buildhost is targeting for Win32.
+else()
+  set_target_properties(flang PROPERTIES VERSION ${FLANG_EXECUTABLE_VERSION})
+endif()
+
+if (FLANG_USE_LEGACY_NAME)
+  set_target_properties(flang PROPERTIES OUTPUT_NAME flang-new)
+endif()
+
 option(FLANG_PLUGIN_SUPPORT "Build Flang with plugin support." ON)
 
-# Enable support for plugins, which need access to symbols from flang-new
+# Enable support for plugins, which need access to symbols from flang
 if(FLANG_PLUGIN_SUPPORT)
-  export_executable_symbols_for_plugins(flang-new)
+  export_executable_symbols_for_plugins(flang)
 endif()
 
-install(TARGETS flang-new DESTINATION "${CMAKE_INSTALL_BINDIR}")
+install(TARGETS flang DESTINATION "${CMAKE_INSTALL_BINDIR}")
Index: flang/tools/f18/CMakeLists.txt
===
--- flang/tools/f18/CMakeLists.txt
+++ flang/tools/f18/CMakeLists.txt
@@ -35,9 +35,9 @@
   endif()
   add_custom_command(OUTPUT ${base}.mod
 COMMAND ${CMAKE_COMMAND} -E make_directory 

[PATCH] D125788: [flang][driver] Rename `flang-new` as `flang`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: clang/lib/Driver/ToolChain.cpp:185
   {"flang", "--driver-mode=flang"},
+  {"flang-new", "--driver-mode=flang"},
   {"clang-dxc", "--driver-mode=dxc"},

clementval wrote:
> Why do we need two lines here? Shouldn't we have a single line with the name 
> chosen by the cmake option?
That would require making this file "configurable" by CMake. That would be 
quite an intrusive design change, which IMO we should avoid.

Unless there's some easy way that I'm missing here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125788

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125788: [flang][driver] Rename `flang-new` as `flang`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D125788#3612533 , @peixin wrote:

> In summary:
>
>> If you want to use the updated name, flang, set FLANG_USE_LEGACY_NAME to ON 
>> when configuring LLVM Flang.
>
> OFF?

Updated, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125788

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128043: [flang][driver] Add support for `-O{0|1|2|3}`

[Andrzej Warzynski via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG869385b11c32: [flang][driver] Add support for `-O{0|1|2|3}` 
(authored by awarzynski).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128043

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/include/flang/Frontend/CodeGenOptions.def
  flang/include/flang/Frontend/CodeGenOptions.h
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/include/flang/Frontend/FrontendActions.h
  flang/lib/Frontend/CMakeLists.txt
  flang/lib/Frontend/CodeGenOptions.cpp
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/test/Driver/default-optimization-pipelines.f90
  flang/test/Driver/driver-help.f90
  flang/test/Driver/flang_f_opts.f90

Index: flang/test/Driver/flang_f_opts.f90
===
--- /dev/null
+++ flang/test/Driver/flang_f_opts.f90
@@ -0,0 +1,14 @@
+! Test for warnings generated when parsing driver options. You can use this file for relatively small tests and to avoid creating
+! new test files.
+
+!---
+! RUN LINES
+!---
+! RUN: %flang -### -S -O4 %s 2>&1 | FileCheck %s
+
+!---
+! EXPECTED OUTPUT
+!---
+! CHECK: warning: -O4 is equivalent to -O3
+! CHECK-LABEL: "-fc1"
+! CHECK: -O3
Index: flang/test/Driver/driver-help.f90
===
--- flang/test/Driver/driver-help.f90
+++ flang/test/Driver/driver-help.f90
@@ -96,6 +96,7 @@
 ! HELP-FC1-NEXT: -fdebug-measure-parse-tree
 ! HELP-FC1-NEXT: Measure the parse tree
 ! HELP-FC1-NEXT: -fdebug-module-writer   Enable debug messages while writing module files
+! HELP-FC1-NEXT: -fdebug-pass-managerPrints debug information for the new pass manage
 ! HELP-FC1-NEXT: -fdebug-pre-fir-treeDump the pre-FIR tree
 ! HELP-FC1-NEXT: -fdebug-unparse-no-sema Unparse and stop (skips the semantic checks)
 ! HELP-FC1-NEXT: -fdebug-unparse-with-symbols
@@ -120,6 +121,7 @@
 ! HELP-FC1-NEXT: -fno-analyzed-objects-for-unparse
 ! HELP-FC1-NEXT:Do not use the analyzed objects when unparsing
 ! HELP-FC1-NEXT: -fno-automatic Implies the SAVE attribute for non-automatic local objects in subprograms unless RECURSIVE
+! HELP-FC1-NEXT: -fno-debug-pass-manager Disables debug printing for the new pass manager
 ! HELP-FC1-NEXT: -fno-reformat  Dump the cooked character stream in -E mode
 ! HELP-FC1-NEXT: -fopenacc  Enable OpenACC
 ! HELP-FC1-NEXT: -fopenmp   Parse OpenMP pragmas and generate parallel code.
Index: flang/test/Driver/default-optimization-pipelines.f90
===
--- /dev/null
+++ flang/test/Driver/default-optimization-pipelines.f90
@@ -0,0 +1,27 @@
+! Verify that`-O{n}` is indeed taken into account when defining the LLVM optimization/middle-end pass pipeline.
+
+!---
+! RUN LINES
+!---
+! RUN: %flang -S -O0 %s -Xflang -fdebug-pass-manager -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-O0
+! RUN: %flang_fc1 -S -O0 %s -fdebug-pass-manager -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-O0
+
+! RUN: %flang -S -O2 %s -Xflang -fdebug-pass-manager -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-O2
+! RUN: %flang_fc1 -S -O2 %s -fdebug-pass-manager -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-O2
+
+!---
+! EXPECTED OUTPUT
+!---
+! CHECK-O0-NOT: Running pass: SimplifyCFGPass on simple_loop_
+! CHECK-O0: Running analysis: TargetLibraryAnalysis on simple_loop_
+
+! CHECK-O2: Running pass: SimplifyCFGPass on simple_loop_
+
+!---
+! INPUT
+!---
+subroutine simple_loop
+  integer :: i
+  do i=1,5
+  end do
+end subroutine
Index: flang/lib/Frontend/FrontendActions.cpp
===
--- flang/lib/Frontend/FrontendActions.cpp
+++ flang/lib/Frontend/FrontendActions.cpp
@@ -46,6 +46,7 @@
 #include "llvm/IRReader/IRReader.h"
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Passes/PassBuilder.h"
+#include "llvm/Passes/StandardInstrumentations.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Target/TargetMachine.h"
@@ -538,7 +539,6 @@
   /*Features=*/"",
   llvm::TargetOptions(), llvm::None));
   assert(tm && "Failed to create TargetMachine");
-  llvmModule->setDataLayout(tm->createDataLayout());
 }
 
 static std::unique_ptr
@@ -610,23 +610,59 @@
   codeGenPasses.run(llvmModule);
 }
 
-/// Generate LLVM byte code file from the input LLVM module.
-///
-/// \param [in] tm Target machine to aid the code-gen pipeline set-up
-/// \param [in] llvmModule LLVM module 

[PATCH] D128043: [flang][driver] Add support for `-O{0|1|2|3}`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski updated this revision to Diff 439285.
awarzynski added a comment.

Add tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128043

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/include/flang/Frontend/CodeGenOptions.def
  flang/include/flang/Frontend/CodeGenOptions.h
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/include/flang/Frontend/FrontendActions.h
  flang/lib/Frontend/CMakeLists.txt
  flang/lib/Frontend/CodeGenOptions.cpp
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/test/Driver/default-optimization-pipelines.f90
  flang/test/Driver/driver-help.f90
  flang/test/Driver/flang_f_opts.f90

Index: flang/test/Driver/flang_f_opts.f90
===
--- /dev/null
+++ flang/test/Driver/flang_f_opts.f90
@@ -0,0 +1,14 @@
+! Test for warnings generated when parsing driver options. You can use this file for relatively small tests and to avoid creating
+! new test files.
+
+!---
+! RUN LINES
+!---
+! RUN: %flang -### -S -O4 %s 2>&1 | FileCheck %s
+
+!---
+! EXPECTED OUTPUT
+!---
+! CHECK: warning: -O4 is equivalent to -O3
+! CHECK-LABEL: "-fc1"
+! CHECK: -O3
Index: flang/test/Driver/driver-help.f90
===
--- flang/test/Driver/driver-help.f90
+++ flang/test/Driver/driver-help.f90
@@ -96,6 +96,7 @@
 ! HELP-FC1-NEXT: -fdebug-measure-parse-tree
 ! HELP-FC1-NEXT: Measure the parse tree
 ! HELP-FC1-NEXT: -fdebug-module-writer   Enable debug messages while writing module files
+! HELP-FC1-NEXT: -fdebug-pass-managerPrints debug information for the new pass manage
 ! HELP-FC1-NEXT: -fdebug-pre-fir-treeDump the pre-FIR tree
 ! HELP-FC1-NEXT: -fdebug-unparse-no-sema Unparse and stop (skips the semantic checks)
 ! HELP-FC1-NEXT: -fdebug-unparse-with-symbols
@@ -120,6 +121,7 @@
 ! HELP-FC1-NEXT: -fno-analyzed-objects-for-unparse
 ! HELP-FC1-NEXT:Do not use the analyzed objects when unparsing
 ! HELP-FC1-NEXT: -fno-automatic Implies the SAVE attribute for non-automatic local objects in subprograms unless RECURSIVE
+! HELP-FC1-NEXT: -fno-debug-pass-manager Disables debug printing for the new pass manager
 ! HELP-FC1-NEXT: -fno-reformat  Dump the cooked character stream in -E mode
 ! HELP-FC1-NEXT: -fopenacc  Enable OpenACC
 ! HELP-FC1-NEXT: -fopenmp   Parse OpenMP pragmas and generate parallel code.
Index: flang/test/Driver/default-optimization-pipelines.f90
===
--- /dev/null
+++ flang/test/Driver/default-optimization-pipelines.f90
@@ -0,0 +1,27 @@
+! Verify that`-O{n}` is indeed taken into account when defining the LLVM optimization/middle-end pass pipeline.
+
+!---
+! RUN LINES
+!---
+! RUN: %flang -S -O0 %s -Xflang -fdebug-pass-manager -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-O0
+! RUN: %flang_fc1 -S -O0 %s -fdebug-pass-manager -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-O0
+
+! RUN: %flang -S -O2 %s -Xflang -fdebug-pass-manager -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-O2
+! RUN: %flang_fc1 -S -O2 %s -fdebug-pass-manager -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-O2
+
+!---
+! EXPECTED OUTPUT
+!---
+! CHECK-O0-NOT: Running pass: SimplifyCFGPass on simple_loop_
+! CHECK-O0: Running analysis: TargetLibraryAnalysis on simple_loop_
+
+! CHECK-O2: Running pass: SimplifyCFGPass on simple_loop_
+
+!---
+! INPUT
+!---
+subroutine simple_loop
+  integer :: i
+  do i=1,5
+  end do
+end subroutine
Index: flang/lib/Frontend/FrontendActions.cpp
===
--- flang/lib/Frontend/FrontendActions.cpp
+++ flang/lib/Frontend/FrontendActions.cpp
@@ -46,6 +46,7 @@
 #include "llvm/IRReader/IRReader.h"
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Passes/PassBuilder.h"
+#include "llvm/Passes/StandardInstrumentations.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Target/TargetMachine.h"
@@ -538,7 +539,6 @@
   /*Features=*/"",
   llvm::TargetOptions(), llvm::None));
   assert(tm && "Failed to create TargetMachine");
-  llvmModule->setDataLayout(tm->createDataLayout());
 }
 
 static std::unique_ptr
@@ -610,23 +610,59 @@
   codeGenPasses.run(llvmModule);
 }
 
-/// Generate LLVM byte code file from the input LLVM module.
-///
-/// \param [in] tm Target machine to aid the code-gen pipeline set-up
-/// \param [in] llvmModule LLVM module to lower to assembly/machine-code
-/// \param [out] os Output stream to emit the generated 

[PATCH] D128043: [flang][driver] Add support for `-O{0|1|2|3}`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D128043#3604037 , @rovka wrote:

> I just realized I haven't pestered you enough about testing :)

I did feel like I was missing something here, haha! Updates arriving shortly!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128043

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128043: [flang][driver] Add support for `-O{0|1|2|3}`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski updated this revision to Diff 439086.
awarzynski added a comment.

Use `D` instead of `TC.getDriver()`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128043

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/include/flang/Frontend/CodeGenOptions.def
  flang/include/flang/Frontend/CodeGenOptions.h
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/include/flang/Frontend/FrontendActions.h
  flang/lib/Frontend/CMakeLists.txt
  flang/lib/Frontend/CodeGenOptions.cpp
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/test/Driver/default-optimization-pipelines.f90
  flang/test/Driver/driver-help.f90

Index: flang/test/Driver/driver-help.f90
===
--- flang/test/Driver/driver-help.f90
+++ flang/test/Driver/driver-help.f90
@@ -96,6 +96,7 @@
 ! HELP-FC1-NEXT: -fdebug-measure-parse-tree
 ! HELP-FC1-NEXT: Measure the parse tree
 ! HELP-FC1-NEXT: -fdebug-module-writer   Enable debug messages while writing module files
+! HELP-FC1-NEXT: -fdebug-pass-managerPrints debug information for the new pass manage
 ! HELP-FC1-NEXT: -fdebug-pre-fir-treeDump the pre-FIR tree
 ! HELP-FC1-NEXT: -fdebug-unparse-no-sema Unparse and stop (skips the semantic checks)
 ! HELP-FC1-NEXT: -fdebug-unparse-with-symbols
@@ -120,6 +121,7 @@
 ! HELP-FC1-NEXT: -fno-analyzed-objects-for-unparse
 ! HELP-FC1-NEXT:Do not use the analyzed objects when unparsing
 ! HELP-FC1-NEXT: -fno-automatic Implies the SAVE attribute for non-automatic local objects in subprograms unless RECURSIVE
+! HELP-FC1-NEXT: -fno-debug-pass-manager Disables debug printing for the new pass manager
 ! HELP-FC1-NEXT: -fno-reformat  Dump the cooked character stream in -E mode
 ! HELP-FC1-NEXT: -fopenacc  Enable OpenACC
 ! HELP-FC1-NEXT: -fopenmp   Parse OpenMP pragmas and generate parallel code.
Index: flang/test/Driver/default-optimization-pipelines.f90
===
--- /dev/null
+++ flang/test/Driver/default-optimization-pipelines.f90
@@ -0,0 +1,24 @@
+! Verify that`-O{n}` is indeed taken into account when defining the LLVM optimization/middle-end pass pass pipeline.
+
+!---
+! RUN LINES
+!---
+! RUN: %flang_fc1 -S -O0 %s -fdebug-pass-manager -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-O0
+! RUN: %flang_fc1 -S -O2 %s -fdebug-pass-manager -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-O2
+
+!---
+! EXPECTED OUTPUT
+!---
+! CHECK-O0-NOT: Running pass: SimplifyCFGPass on simple_loop_
+! CHECK-O0: Running analysis: TargetLibraryAnalysis on simple_loop_
+
+! CHECK-O2: Running pass: SimplifyCFGPass on simple_loop_
+
+!---
+! INPUT
+!---
+subroutine simple_loop
+  integer :: i
+  do i=1,5
+  end do
+end subroutine
Index: flang/lib/Frontend/FrontendActions.cpp
===
--- flang/lib/Frontend/FrontendActions.cpp
+++ flang/lib/Frontend/FrontendActions.cpp
@@ -46,6 +46,7 @@
 #include "llvm/IRReader/IRReader.h"
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Passes/PassBuilder.h"
+#include "llvm/Passes/StandardInstrumentations.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Target/TargetMachine.h"
@@ -538,7 +539,6 @@
   /*Features=*/"",
   llvm::TargetOptions(), llvm::None));
   assert(tm && "Failed to create TargetMachine");
-  llvmModule->setDataLayout(tm->createDataLayout());
 }
 
 static std::unique_ptr
@@ -610,23 +610,59 @@
   codeGenPasses.run(llvmModule);
 }
 
-/// Generate LLVM byte code file from the input LLVM module.
-///
-/// \param [in] tm Target machine to aid the code-gen pipeline set-up
-/// \param [in] llvmModule LLVM module to lower to assembly/machine-code
-/// \param [out] os Output stream to emit the generated code to
-static void generateLLVMBCImpl(llvm::TargetMachine ,
-   llvm::Module ,
-   llvm::raw_pwrite_stream ) {
-  // Set-up the pass manager
-  llvm::ModulePassManager mpm;
+static llvm::OptimizationLevel
+mapToLevel(const Fortran::frontend::CodeGenOptions ) {
+  switch (opts.OptimizationLevel) {
+  default:
+llvm_unreachable("Invalid optimization level!");
+  case 0:
+return llvm::OptimizationLevel::O0;
+  case 1:
+return llvm::OptimizationLevel::O1;
+  case 2:
+return llvm::OptimizationLevel::O2;
+  case 3:
+return llvm::OptimizationLevel::O3;
+  }
+}
+
+void CodeGenAction::runOptimizationPipeline(llvm::raw_pwrite_stream ) {
+  auto opts = getInstance().getInvocation().getCodeGenOpts();
+  llvm::OptimizationLevel 

[PATCH] D128043: [flang][driver] Add support for `-O{0|1|2|3}`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:133
+  CmdArgs.push_back("-O3");
+  TC.getDriver().Diag(diag::warn_O4_is_O3);
+} else {

peixin wrote:
> Nit: I have committed D126164, and you can rebase and use D directly, which 
> is the style in `Clang.cpp`.
Thanks for the heads up and for seeing https://reviews.llvm.org/D126164 
through! I'll update it now before I forget :)



Comment at: flang/include/flang/Frontend/CodeGenOptions.def:12
+// Optionally, the user may also define ENUM_CODEGENOPT (for options
+// that have enumeration type and VALUE_CODEGENOPT is a code
+// generation option that describes a value rather than a flag.

rovka wrote:
> I'm not sure I understand the difference between CODEGENOPT and 
> VALUE_CODEGENOPT. Is it that CODEGENOPT is actually a kind of 
> BOOL_CODEGENOPT, that should always have just 1 bit? Do we really need both?
At one point I convinved myself that I understand the difference, but now I'm 
re-rereading Clang's [[ 
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/CodeGenOptions.def
 | CodeGenOptions.def ]] and I also see ... no difference   .

Thanks for pointing this out! Let me remove it.



Comment at: flang/include/flang/Frontend/CodeGenOptions.def:25
+
+#ifndef ENUM_CODEGENOPT
+#  define ENUM_CODEGENOPT(Name, Type, Bits, Default) \

rovka wrote:
> This isn't used yet, can we skip it?
Yup, good suggestion!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128043

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128043: [flang][driver] Add support for `-O{0|1|2|3}`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski updated this revision to Diff 438995.
awarzynski marked 3 inline comments as done.
awarzynski added a comment.

Address comments from Peixin and Diana

Main change - removed ENUM_CODEGENOPT and VALUE_CODEGENOPT from 
CodeGenOptions.dev.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128043

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/include/flang/Frontend/CodeGenOptions.def
  flang/include/flang/Frontend/CodeGenOptions.h
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/include/flang/Frontend/FrontendActions.h
  flang/lib/Frontend/CMakeLists.txt
  flang/lib/Frontend/CodeGenOptions.cpp
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/test/Driver/default-optimization-pipelines.f90
  flang/test/Driver/driver-help.f90

Index: flang/test/Driver/driver-help.f90
===
--- flang/test/Driver/driver-help.f90
+++ flang/test/Driver/driver-help.f90
@@ -95,6 +95,7 @@
 ! HELP-FC1-NEXT: -fdebug-measure-parse-tree
 ! HELP-FC1-NEXT: Measure the parse tree
 ! HELP-FC1-NEXT: -fdebug-module-writer   Enable debug messages while writing module files
+! HELP-FC1-NEXT: -fdebug-pass-managerPrints debug information for the new pass manage
 ! HELP-FC1-NEXT: -fdebug-pre-fir-treeDump the pre-FIR tree
 ! HELP-FC1-NEXT: -fdebug-unparse-no-sema Unparse and stop (skips the semantic checks)
 ! HELP-FC1-NEXT: -fdebug-unparse-with-symbols
@@ -119,6 +120,7 @@
 ! HELP-FC1-NEXT: -fno-analyzed-objects-for-unparse
 ! HELP-FC1-NEXT:Do not use the analyzed objects when unparsing
 ! HELP-FC1-NEXT: -fno-automatic Implies the SAVE attribute for non-automatic local objects in subprograms unless RECURSIVE
+! HELP-FC1-NEXT: -fno-debug-pass-manager Disables debug printing for the new pass manager
 ! HELP-FC1-NEXT: -fno-reformat  Dump the cooked character stream in -E mode
 ! HELP-FC1-NEXT: -fopenacc  Enable OpenACC
 ! HELP-FC1-NEXT: -fopenmp   Parse OpenMP pragmas and generate parallel code.
Index: flang/test/Driver/default-optimization-pipelines.f90
===
--- /dev/null
+++ flang/test/Driver/default-optimization-pipelines.f90
@@ -0,0 +1,24 @@
+! Verify that`-O{n}` is indeed taken into account when defining the LLVM optimization/middle-end pass pass pipeline.
+
+!---
+! RUN LINES
+!---
+! RUN: %flang_fc1 -S -O0 %s -fdebug-pass-manager -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-O0
+! RUN: %flang_fc1 -S -O2 %s -fdebug-pass-manager -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-O2
+
+!---
+! EXPECTED OUTPUT
+!---
+! CHECK-O0-NOT: Running pass: SimplifyCFGPass on simple_loop_
+! CHECK-O0: Running analysis: TargetLibraryAnalysis on simple_loop_
+
+! CHECK-O2: Running pass: SimplifyCFGPass on simple_loop_
+
+!---
+! INPUT
+!---
+subroutine simple_loop
+  integer :: i
+  do i=1,5
+  end do
+end subroutine
Index: flang/lib/Frontend/FrontendActions.cpp
===
--- flang/lib/Frontend/FrontendActions.cpp
+++ flang/lib/Frontend/FrontendActions.cpp
@@ -46,6 +46,7 @@
 #include "llvm/IRReader/IRReader.h"
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Passes/PassBuilder.h"
+#include "llvm/Passes/StandardInstrumentations.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Target/TargetMachine.h"
@@ -538,7 +539,6 @@
   /*Features=*/"",
   llvm::TargetOptions(), llvm::None));
   assert(tm && "Failed to create TargetMachine");
-  llvmModule->setDataLayout(tm->createDataLayout());
 }
 
 static std::unique_ptr
@@ -610,23 +610,59 @@
   codeGenPasses.run(llvmModule);
 }
 
-/// Generate LLVM byte code file from the input LLVM module.
-///
-/// \param [in] tm Target machine to aid the code-gen pipeline set-up
-/// \param [in] llvmModule LLVM module to lower to assembly/machine-code
-/// \param [out] os Output stream to emit the generated code to
-static void generateLLVMBCImpl(llvm::TargetMachine ,
-   llvm::Module ,
-   llvm::raw_pwrite_stream ) {
-  // Set-up the pass manager
-  llvm::ModulePassManager mpm;
+static llvm::OptimizationLevel
+mapToLevel(const Fortran::frontend::CodeGenOptions ) {
+  switch (opts.OptimizationLevel) {
+  default:
+llvm_unreachable("Invalid optimization level!");
+  case 0:
+return llvm::OptimizationLevel::O0;
+  case 1:
+return llvm::OptimizationLevel::O1;
+  case 2:
+return llvm::OptimizationLevel::O2;
+  case 3:
+return llvm::OptimizationLevel::O3;
+  }
+}
+
+void 

[PATCH] D128043: [flang][driver] Add support for `-O{0|1|2|3}`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for taking a look, @peixin! Just to clarify - I'm not really looking 
into `-Os`, `-Ofast` or `-Oz` at the moment. But I'm always happy to review 
driver patches :)




Comment at: clang/include/clang/Driver/Options.td:732
+def O_flag : Flag<["-"], "O">, Flags<[CC1Option,FC1Option]>, Alias, 
AliasArgs<["1"]>;
 def Ofast : Joined<["-"], "Ofast">, Group, Flags<[CC1Option]>;
 def P : Flag<["-"], "P">, Flags<[CC1Option,FlangOption,FC1Option]>, 
Group,

peixin wrote:
> Will enabling Ofast require more changes in the flang frontend? If yes, it is 
> OK to support it in another patch later.
I would much prefer to have it implemented it in a dedicated patch.

For every option, one has to consider the semantics in the compiler driver 
(`flang-new`) vs frontend driver (`flang-new -fc1`) and then, in case of 
code-gen flags, the difference between middle-end and back-end pass pipelines. 
So quite a few things :)

In general, I'm in favor of doing this incrementally, in small patches. This 
makes reviewing and testing easier and more self-contained. Is that OK?



Comment at: flang/include/flang/Frontend/CodeGenOptions.def:30
+
+VALUE_CODEGENOPT(OptimizationLevel, 2, 0) ///< The -O[0-3] option specified.
+

peixin wrote:
> I saw `clang/include/clang/Basic/CodeGenOptions.def` has the following:
> ```
> VALUE_CODEGENOPT(OptimizationLevel, 2, 0) ///< The -O[0-3] option specified.
> VALUE_CODEGENOPT(OptimizeSize, 2, 0) ///< If -Os (==1) or -Oz (==2) is 
> specified.
> ```
> 
> Do `-Os` and `-Oz` need extra processing in flang drivers? If yes, it is OK 
> to support it in another patch later.
> Do -Os and -Oz need extra processing in flang drivers? 

These are code-size optimisation flags, so the logic will be a bit different.

Also, I'm very much in favor of small, incremental and self-contained patches. 
Splitting this into regular and size optimizations seemed like a natural split.



Comment at: flang/lib/Frontend/CodeGenOptions.cpp:8
+//===--===//
+
+#include "flang/Frontend/CodeGenOptions.h"

peixin wrote:
> Miss the following?
> ```
> //
> // Coding style: https://mlir.llvm.org/getting_started/DeveloperGuide/
> //
> //===--===//
> ```
> 
Good catch, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128043

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128333: [clang][flang] Disable defaulting to `-fpie` for LLVM Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D128333#3601299 , 
@kiranchandramohan wrote:

> Is the longer-term plan to support this in Flang as well?

I don't see why not. AFAIK, the switch in Clang took a while and happened 
gradually - so we probably shouldn't rush this in Flang. We'd also need to 
check the behavior in `gfortran` (so that we maintain consistency).

> Does this affect building object files too or is it just the executable?

Just the executable.

AFAIK, `isPIEDefault` is only used in tools::ParsePICArgs 

 . In Flang, we don't support any of `-fpic/-fPIC/-fpie/-fPIE`. And 
`CLANG_DEFAULT_PIE_ON_LINUX` leads to `-fpie` being added by default to the 
final linker invocation (when generating the executable). But that doesn't make 
sense if the object files were generated with `-fpic`. Hence the change.

> How would this affect mixed C++/Fortran programs if the Clang and Flang 
> settings are different?

Good question! We've not experimented with mixed C++/Fortran programs enough, 
so I don't have an answer. But we should probably create a GitHub ticket to 
restore consistency between Clang and Flang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128333

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128333: [clang][flang] Disable defaulting to `-fpie` for LLVM Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski created this revision.
awarzynski added reviewers: rovka, MaskRay, schweitz, Leporacanthicus.
Herald added subscribers: jsji, StephenFan, pengfei, kristof.beyls.
Herald added a reviewer: sscalpone.
Herald added projects: Flang, All.
awarzynski requested review of this revision.
Herald added subscribers: cfe-commits, jdoerfert.
Herald added a project: clang.

In, https://reviews.llvm.org/D120305, CLANG_DEFAULT_PIE_ON_LINUX was set
to `On` by default. However, neither `-fpie` nor `-fpic` are currently
supported in LLVM Flang. Hence, in this patch the behaviour controlled
with CLANG_DEFAULT_PIE_ON_LINUX is refined not to apply to Flang.

Another way to look at this is that CLANG_DEFAULT_PIE_ON_LINUX is
currently affecting both Clang and Flang. IIUC, the intention for this
CMake variable has always been to only affect Clang. This patch makes
sure that that's the case.

Without this change, you might see errors like this on X86_64:

  /usr/bin/ld: main.o: relocation R_X86_64_32 against `.bss' can not be used 
when making a PIE object; recompile with -fPIC

I've not experienced any issues on AArch64. That's probably because on
AArch64 some object files happen to be position independent without
needing -fpie or -fpic.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128333

Files:
  clang/lib/Driver/ToolChains/Linux.cpp
  flang/test/Driver/no-pie.f90


Index: flang/test/Driver/no-pie.f90
===
--- /dev/null
+++ flang/test/Driver/no-pie.f90
@@ -0,0 +1,12 @@
+! Verify that in contrast to Clang, Flang does not default to generating 
position independent executables/code
+
+!-
+! RUN COMMANDS
+!-
+! RUN: %flang -### -flang-experimental-exec %S/Inputs/hello.f90 2>&1 | 
FileCheck %s
+
+!
+! EXPECTED OUTPUT
+!
+! CHECK-NOT: fpie
+! CHECK-NOT: fpic
Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -699,8 +699,9 @@
 }
 
 bool Linux::isPIEDefault(const llvm::opt::ArgList ) const {
-  return CLANG_DEFAULT_PIE_ON_LINUX || getTriple().isAndroid() ||
- getTriple().isMusl() || getSanitizerArgs(Args).requiresPIE();
+  return !getDriver().IsFlangMode() &&
+ (CLANG_DEFAULT_PIE_ON_LINUX || getTriple().isAndroid() ||
+  getTriple().isMusl() || getSanitizerArgs(Args).requiresPIE());
 }
 
 bool Linux::IsAArch64OutlineAtomicsDefault(const ArgList ) const {


Index: flang/test/Driver/no-pie.f90
===
--- /dev/null
+++ flang/test/Driver/no-pie.f90
@@ -0,0 +1,12 @@
+! Verify that in contrast to Clang, Flang does not default to generating position independent executables/code
+
+!-
+! RUN COMMANDS
+!-
+! RUN: %flang -### -flang-experimental-exec %S/Inputs/hello.f90 2>&1 | FileCheck %s
+
+!
+! EXPECTED OUTPUT
+!
+! CHECK-NOT: fpie
+! CHECK-NOT: fpic
Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -699,8 +699,9 @@
 }
 
 bool Linux::isPIEDefault(const llvm::opt::ArgList ) const {
-  return CLANG_DEFAULT_PIE_ON_LINUX || getTriple().isAndroid() ||
- getTriple().isMusl() || getSanitizerArgs(Args).requiresPIE();
+  return !getDriver().IsFlangMode() &&
+ (CLANG_DEFAULT_PIE_ON_LINUX || getTriple().isAndroid() ||
+  getTriple().isMusl() || getSanitizerArgs(Args).requiresPIE());
 }
 
 bool Linux::IsAArch64OutlineAtomicsDefault(const ArgList ) const {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128043: [flang][driver] Add support for `-O{0|1|2|3}`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D128043#3596096 , @peixin wrote:

> The CI failed.

Thanks - I didn't notice any failures related to this change. Did I miss 
anything?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128043

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127207: [flang][driver] Fix support for `-x`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D127207#3593665 , @sunho wrote:

> Hi! I'm not exactly sure what's going on. But, seems like build is failing 
> here? https://lab.llvm.org/buildbot/#/builders/177/builds/5571

Thanks for flagging this up! I am also rather confused. I checked the list of 
changes for that buildbot job  and it all looks totally unrelated to `flang-new 
-x`. The error is:

   TEST 'Flang :: Driver/input-from-stdin-llvm.ll' FAILED 

  Script:
  --
  : 'RUN: at line 10';   cat 
/home/tcwg-buildbot/worker/flang-aarch64-dylib/llvm-project/flang/test/Driver/input-from-stdin-llvm.ll
 | /home/tcwg-buildbot/worker/flang-aarch64-dylib/build/bin/not 
/home/tcwg-buildbot/worker/flang-aarch64-dylib/build/bin/flang-new -S - -o -
  : 'RUN: at line 11';   cat 
/home/tcwg-buildbot/worker/flang-aarch64-dylib/llvm-project/flang/test/Driver/input-from-stdin-llvm.ll
 | /home/tcwg-buildbot/worker/flang-aarch64-dylib/build/bin/not 
/home/tcwg-buildbot/worker/flang-aarch64-dylib/build/bin/flang-new -fc1 -S - -o 
-
  : 'RUN: at line 14';   cat 
/home/tcwg-buildbot/worker/flang-aarch64-dylib/llvm-project/flang/test/Driver/input-from-stdin-llvm.ll
 | /home/tcwg-buildbot/worker/flang-aarch64-dylib/build/bin/flang-new -x ir -S 
-target aarch64-unknown-linux-gnu - -o - | 
/home/tcwg-buildbot/worker/flang-aarch64-dylib/build/bin/FileCheck 
/home/tcwg-buildbot/worker/flang-aarch64-dylib/llvm-project/flang/test/Driver/input-from-stdin-llvm.ll
  : 'RUN: at line 15';   cat 
/home/tcwg-buildbot/worker/flang-aarch64-dylib/llvm-project/flang/test/Driver/input-from-stdin-llvm.ll
 | /home/tcwg-buildbot/worker/flang-aarch64-dylib/build/bin/flang-new -fc1 -x 
ir -S -triple aarch64-unknown-linux-gnu - -o - | 
/home/tcwg-buildbot/worker/flang-aarch64-dylib/build/bin/FileCheck 
/home/tcwg-buildbot/worker/flang-aarch64-dylib/llvm-project/flang/test/Driver/input-from-stdin-llvm.ll
  --
  Exit Code: 141
  Command Output (stderr):
  --
  + : 'RUN: at line 10'
  + /home/tcwg-buildbot/worker/flang-aarch64-dylib/build/bin/not 
/home/tcwg-buildbot/worker/flang-aarch64-dylib/build/bin/flang-new -S - -o -
  + cat 
/home/tcwg-buildbot/worker/flang-aarch64-dylib/llvm-project/flang/test/Driver/input-from-stdin-llvm.ll
  error: Could not scan -
  standard input:10:15: error: bad character ('|') in Fortran token
; RUN: cat %s | not %flang -S - -o -
  ^
  standard input:11:15: error: bad character ('|') in Fortran token
; RUN: cat %s | not %flang_fc1 -S - -o -
  ^
  standard input:14:15: error: bad character ('|') in Fortran token
; RUN: cat %s | %flang -x ir -S -target aarch64-unknown-linux-gnu - -o - | 
FileCheck %s
  ^
  standard input:14:74: error: bad character ('|') in Fortran token
; RUN: cat %s | %flang -x ir -S -target aarch64-unknown-linux-gnu - -o - | 
FileCheck %s
 ^
  standard input:15:15: error: bad character ('|') in Fortran token
; RUN: cat %s | %flang_fc1 -x ir -S -triple aarch64-unknown-linux-gnu - -o 
- | FileCheck %s
  ^
  standard input:15:78: error: bad character ('|') in Fortran token
; RUN: cat %s | %flang_fc1 -x ir -S -triple aarch64-unknown-linux-gnu - -o 
- | FileCheck %s

 ^
  standard input:26:20: error: bad character ('{') in Fortran token
define void @foo() {
   ^
  standard input:28:1: error: bad character ('}') in Fortran token
}
^
  + : 'RUN: at line 11'
  + /home/tcwg-buildbot/worker/flang-aarch64-dylib/build/bin/not 
/home/tcwg-buildbot/worker/flang-aarch64-dylib/build/bin/flang-new -fc1 -S - -o 
-
  + cat 
/home/tcwg-buildbot/worker/flang-aarch64-dylib/llvm-project/flang/test/Driver/input-from-stdin-llvm.ll
  error: Invalid input type - expecting a Fortran file
  --
  

But lines 10 and 11 in input-from-stdin-llvm.ll 

 //are expected to fail// and hence there's `not %flang` rather than `%flang` 樂 
. The subsequent buildbot job 
 is fine, so probably 
no need to change anything just now. But I will monitor this!

Thanks again for letting me know!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127207

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128043: [flang][driver] Add support for `-O{0|1|2|3}`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski created this revision.
awarzynski added reviewers: rovka, kiranchandramohan, schweitz, peixin.
Herald added subscribers: bzcheeseman, rriddle, mgorny.
Herald added a reviewer: sscalpone.
Herald added projects: Flang, All.
awarzynski requested review of this revision.
Herald added subscribers: cfe-commits, stephenneuendorffer, jdoerfert, MaskRay.
Herald added a project: clang.

This patch adds support for most common optimisation compiler flags:
`-O{0|1|2|3}`. This is implemented in both the compiler and fronted
drivers. At this point, these options are only used to configure the
LLVM optimisation pipelines (aka middle-end). LLVM backend or MLIR/FIR
optimisations are not supported yet.

Previously, the middle-end pass manager was only required when
generating LLVM bitcode (i.e. for `flang-new -c -emit-llvm ` or
`flang-new -fc1 -emit-llvm-bc `). With this change, it becomes
required for all frontend actions that are represented as
`CodeGenAction` and `CodeGenAction::executeAction` is refactored
accordingly (in the spirit of better code re-use).

Additionally, the `-fdebug-pass-manager` option is enabled to facilitate
testing. This flag can be used to configure the pass manager to print
the middle-end passes that are being run. Similar option exists in Clang
and the semantics in Flang are identical. This option translates to
extra configuration when setting up the pass manager. This is
implemented in `CodeGenAction::runOptimizationPipeline`.

This patch also adds some bolier plate code to manage code-gen options
("code-gen" refers to generating machine code in LLVM in this context).
This was extracted from Clang. In Clang, it simplifies defining code-gen
options and enables option marshalling. In Flang, option marshalling is
not yet supported (we might do at some point), but being able to
auto-generate some code with macros is beneficial. This will become
particularly apparent when we start adding more options (at least in
Clang, the list of code-gen options is rather long).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128043

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/include/flang/Frontend/CodeGenOptions.def
  flang/include/flang/Frontend/CodeGenOptions.h
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/include/flang/Frontend/FrontendActions.h
  flang/lib/Frontend/CMakeLists.txt
  flang/lib/Frontend/CodeGenOptions.cpp
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/test/Driver/default-optimization-pipelines.f90
  flang/test/Driver/driver-help.f90

Index: flang/test/Driver/driver-help.f90
===
--- flang/test/Driver/driver-help.f90
+++ flang/test/Driver/driver-help.f90
@@ -95,6 +95,7 @@
 ! HELP-FC1-NEXT: -fdebug-measure-parse-tree
 ! HELP-FC1-NEXT: Measure the parse tree
 ! HELP-FC1-NEXT: -fdebug-module-writer   Enable debug messages while writing module files
+! HELP-FC1-NEXT: -fdebug-pass-managerPrints debug information for the new pass manage
 ! HELP-FC1-NEXT: -fdebug-pre-fir-treeDump the pre-FIR tree
 ! HELP-FC1-NEXT: -fdebug-unparse-no-sema Unparse and stop (skips the semantic checks)
 ! HELP-FC1-NEXT: -fdebug-unparse-with-symbols
@@ -119,6 +120,7 @@
 ! HELP-FC1-NEXT: -fno-analyzed-objects-for-unparse
 ! HELP-FC1-NEXT:Do not use the analyzed objects when unparsing
 ! HELP-FC1-NEXT: -fno-automatic Implies the SAVE attribute for non-automatic local objects in subprograms unless RECURSIVE
+! HELP-FC1-NEXT: -fno-debug-pass-manager Disables debug printing for the new pass manager
 ! HELP-FC1-NEXT: -fno-reformat  Dump the cooked character stream in -E mode
 ! HELP-FC1-NEXT: -fopenacc  Enable OpenACC
 ! HELP-FC1-NEXT: -fopenmp   Parse OpenMP pragmas and generate parallel code.
Index: flang/test/Driver/default-optimization-pipelines.f90
===
--- /dev/null
+++ flang/test/Driver/default-optimization-pipelines.f90
@@ -0,0 +1,24 @@
+! Verify that`-O{n}` is indeed taken into account when definining the LLVM optimization/middle-end pass pass pipeline.
+
+!---
+! RUN LINES
+!---
+! RUN: %flang_fc1 -S -O0 %s -fdebug-pass-manager -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-O0
+! RUN: %flang_fc1 -S -O2 %s -fdebug-pass-manager -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-O2
+
+!---
+! EXPECTED OUTPUT
+!---
+! CHECK-O0-NOT: Running pass: SimplifyCFGPass on simple_loop_
+! CHECK-O0: Running analysis: TargetLibraryAnalysis on simple_loop_
+
+! CHECK-O2: Running pass: SimplifyCFGPass on simple_loop_
+
+!---
+! INPUT
+!---
+subroutine simple_loop
+  integer :: i
+  do i=1,5
+  end do
+end subroutine
Index: flang/lib/Frontend/FrontendActions.cpp

[PATCH] D126291: [flang][Driver] Update link job on windows

[Andrzej Warzynski via Phabricator via cfe-commits]

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

LGTM, many thanks for this non trivial effort! :) I've left a few nits, feel 
free to ignore! @mstorsjo , are you also OK with this change?

[nit] "This is exactly what we do for Linux/Darwin, but the interface is 
slightly different (e.g. -libpath instead of -L)." -> Perhaps clarify what 
"interface" you have in mind (e.g. "Windows interface").




Comment at: flang/test/Driver/linker-flags.f90:12
 
-!
-! RUN COMMAND
-!
-! Use `--ld-path` so that the linker location (used in the LABEL below) is 
deterministic.
-! RUN: %flang -### -flang-experimental-exec --ld-path=/usr/bin/ld 
%S/Inputs/hello.f90 2>&1 | FileCheck %s
+! NOTE: Clang usually adds 'libcmt' and 'oldnames' on Windows, but
+!   they are not needed when compiling Fortran code and they might

This is a nit.


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126291: [flang][Driver] Update link job on windows

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/test/Driver/linker-flags.f90:51
+! MSVC-NOT: libcmt
+! MSVC-NOT: oldnames
+! MSVC-SAME: "[[object_file]]"

rovka wrote:
> awarzynski wrote:
> > rovka wrote:
> > > mmuetzel wrote:
> > > > Lines 50-51 seem to be duplicates of lines 44-45. Is this intentional?
> > > Yes, I don't want those to appear either before or after the Fortran 
> > > libs. I guess if we wanted to be pedantic we'd also check that they don't 
> > > appear after the object_file, or between the libs and the subsystem, but 
> > > that seems a bit much.
> > Based on the [[ 
> > https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-same-directive 
> > | docs ]], I'd say that this would be the idiomatic way to do this:
> > ```lang=bash
> > ! MSVC-LABEL:
> > ! MSVC-NOT: 
> > ! MSVC-SAME:
> > ```
> > IIUC, the following would only be needed if there's a potential for 
> > `libcmt` or `oldnames` to appear on a separate line:
> > ```
> > ```lang=bash
> > ! MSVC-LABEL:
> > ! MSVC-NOT: 
> > ! MSVC-SAME:
> > ! MSVC-NOT:
> > ```
> > But this wouldn't happen, right? (there's going to be only one linker 
> > invocation). Also, you could just use [[ 
> > https://llvm.org/docs/CommandGuide/FileCheck.html#options | 
> > --implicit-check-not ]] :)
> > Based on the [[ 
> > https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-same-directive 
> > | docs ]], I'd say that this would be the idiomatic way to do this:
> > ```lang=bash
> > ! MSVC-LABEL:
> > ! MSVC-NOT: 
> > ! MSVC-SAME:
> > ```
> 
> Based on the same docs, I would say it shouldn't be enough to mention it just 
> once. But that's just what I expect, the docs are completely unhelpful about 
> the actual behaviour here. For instance, I would expect to be able to write
> ```
> ! MSVC-NOT: should-only-come-after-X
> ! MSVC-SAME: X
> ```
> If the MSVC-NOT applies to the whole line, then lines with 'X 
> should-come-after-X' get rejected, but imo they should be accepted (I'll 
> admit I didn't actually verify this, and anyway the implicit-check-not makes 
> the whole discussion moot).
> 
> > IIUC, the following would only be needed if there's a potential for 
> > `libcmt` or `oldnames` to appear on a separate line:
> > ```
> > ```lang=bash
> > ! MSVC-LABEL:
> > ! MSVC-NOT: 
> > ! MSVC-SAME:
> > ! MSVC-NOT:
> > ```
> 
> I agree that if there's no MSVC-SAME after the last MSVC-NOT, then the 
> MSVC-NOT would apply to the following line. 
> 
> > But this wouldn't happen, right? (there's going to be only one linker 
> > invocation). Also, you could just use [[ 
> > https://llvm.org/docs/CommandGuide/FileCheck.html#options | 
> > --implicit-check-not ]] :)
> 
> Wooow  I didn't know about that one, I'll definitely update the test to use 
> it, thanks! 
> Based on the same docs, I would say it shouldn't be enough to mention it just 
> once. 

I'm re-rereading the docs and agree. Sounds like `--implicit-check-not` is the 
best option.

> For instance, I would expect to be able to write
> ```
> ! MSVC-NOT: should-only-come-after-X
> ! MSVC-SAME: X
> ```
> If the MSVC-NOT applies to the whole line, then lines with 'X 
> should-come-after-X' get rejected

I think that it //should// and //is// accepted. Example below:

**file.f90**
```lang=bash
! RUN: cat %S/test.f90 | FileCheck %s

! CHECK-LABEL: my-label
! CHECK-NOT: should-only-come-after-X
! CHECK-SAME: X
! CHECK-SAME: should-only-come-after-X
```

**test.f90**
```lang=bash
my-label X should-only-come-after-X
```
I copied the above into the Driver test dir and run like this:
```
$ bin/llvm-lit -va ../../flang/test/Driver/file.f90
-- Testing: 1 tests, 1 workers --
PASS: Flang :: Driver/file.f90 (1 of 1)

Testing Time: 0.03s
  Passed: 1
```

Does this agree with your experiments?

#filecheck-is-confusing :)


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126291: [flang][Driver] Update link job on windows

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/test/Driver/linker-flags.f90:51
+! MSVC-NOT: libcmt
+! MSVC-NOT: oldnames
+! MSVC-SAME: "[[object_file]]"

rovka wrote:
> mmuetzel wrote:
> > Lines 50-51 seem to be duplicates of lines 44-45. Is this intentional?
> Yes, I don't want those to appear either before or after the Fortran libs. I 
> guess if we wanted to be pedantic we'd also check that they don't appear 
> after the object_file, or between the libs and the subsystem, but that seems 
> a bit much.
Based on the [[ 
https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-same-directive | 
docs ]], I'd say that this would be the idiomatic way to do this:
```lang=bash
! MSVC-LABEL:
! MSVC-NOT: 
! MSVC-SAME:
```
IIUC, the following would only be needed if there's a potential for `libcmt` or 
`oldnames` to appear on a separate line:
```
```lang=bash
! MSVC-LABEL:
! MSVC-NOT: 
! MSVC-SAME:
! MSVC-NOT:
```
But this wouldn't happen, right? (there's going to be only one linker 
invocation). Also, you could just use [[ 
https://llvm.org/docs/CommandGuide/FileCheck.html#options | 
--implicit-check-not ]] :)


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126291: [flang][Driver] Update link job on windows

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/test/Driver/linker-flags.f90:28
+
+! GNU-WITHOUTLM-LABEL:  "{{.*}}ld" 
+! GNU-WITHOUTLM-SAME: "[[object_file]]"

I think that GNU in this case might be a bit misleading. These linker 
invocations are defined in almost completely independent toolchains: [[ 
https://github.com/llvm/llvm-project/blob/b9a7dea9171416a998e4fafb9f76baa167b8/clang/lib/Driver/ToolChains/Darwin.h#L33-L47
 | MachOTool ]], [[ 
https://github.com/llvm/llvm-project/blob/b9a7dea9171416a998e4fafb9f76baa167b8/clang/lib/Driver/ToolChains/Gnu.h#L40-L51
 | gnutools ]].

I't be inclined to try this instead:
```
! RUN: %flang -### -flang-experimental-exec -target ppc64le-linux-gnu 
%S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,GNU
! RUN: %flang -### -flang-experimental-exec -target aarch64-apple-darwin 
%S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,DARWIN
! RUN: %flang -### -flang-experimental-exec -target x86_64-windows-gnu 
%S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,MINGW
! RUN: %flang -### -flang-experimental-exec -target aarch64-windows-msvc 
%S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,MSVC
```

This will lead to more duplication, but would clarify things a bit.


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126291: [flang][Driver] Update link job on windows

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

樂 linker-flags.f90 is failing on Windows in the pre-commit CI. Not sure why - 
seems fine on Debian.




Comment at: flang/test/Driver/linker-flags.f90:28
+! GNU-SAME: -lFortranDecimal
+! WITHLM-SAME: -lm
+

Does `-SAME` makese sense here? As in, this makes sense to me:
```
! GNU: -lFortranDecimal
! GNU-SAME: -lm
```
and this:
```
! WITHLM: -lFortranDecimal
! WITHLM-SAME: -lm
```
but for `! WITHLM-SAME: -lm` there's no [[ 
https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-same-directive | 
previous match ]], is there?



Comment at: flang/test/Driver/linker-flags.f90:33-34
+! MSVC-LABEL: link.exe
+! MSVC-NOT: libcmt
+! MSVC-NOT: oldnames
+! MSVC-SAME: Fortran_main.lib

Is it worth adding a comment to explain why to single these out?


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits