[PATCH] D136090: Handle errors in expansion of response files

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

Nice tests! :) I have one microscopic nit and one question (because I don't 
know an awful lot about config files). Thanks for working on this.




Comment at: llvm/lib/Support/CommandLine.cpp:1188
+  // macros.
+  if (!RelativeNames && !InConfigFile)
 return Error::success();

Why do you need to add `!InConfigFile` here and below? 



Comment at: llvm/lib/Support/CommandLine.cpp:1204
+StringRef FileName;
+if (ArgStr[0] == '@') {
+  FileName = ArgStr.drop_front(1);

Nit: Why reverse this branch? The code around it `continue`s early, so it's 
easier to read if this `continue`s early too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136090

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

[PATCH] D136090: Handle errors in expansion of response files

[Diana Picus via Phabricator via cfe-commits]

rovka added inline comments.



Comment at: clang/test/Driver/config-file-errs.c:16
 // RUN: not %clang --config somewhere/nonexistent-config-file 2>&1 | FileCheck 
%s -check-prefix CHECK-NONEXISTENT
-// CHECK-NONEXISTENT: configuration file 
'{{.*}}somewhere/nonexistent-config-file' does not exist
+// CHECK-NONEXISTENT: configuration file 
'{{.*}}somewhere/nonexistent-config-file' cannot be opened: No such file or 
directory
 

I think you might need to use the same trick as [[ 
https://github.com/llvm/llvm-project/blob/main/flang/test/Driver/output-paths.f90
 | this test ]]  to get the test to pass on both Windows and Linux (since they 
use different capitalization).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136090

___
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

[Diana Picus via Phabricator via cfe-commits]

rovka added inline comments.
Herald added a subscriber: zero9178.



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

jpenix-quic wrote:
> awarzynski wrote:
> > rovka wrote:
> > > Nit: Shouldn't you also check that valid values (e.g. unknown, swap etc) 
> > > are accepted? 
> > 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?
> > Nit: Shouldn't you also check that valid values (e.g. unknown, swap etc) 
> > are accepted? 
> 
> I might be doing something silly/missing something obvious, but I wasn't sure 
> how to get FileCheck to basically just check that the command didn't error 
> without adding checks, so I added trivial checks (VALID/VALID_FC1) for each 
> of unknown/native/etc. Is there a better way of doing this? I also expanded 
> flang/test/Lower/convert.f90 to check lowering for each of the valid options, 
> so I didn't want to do full checks of the MLIR here. 
Just in case it's not clear, this looks good to me now, it's exactly what I had 
in mind.


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] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

IMO this is a reasonable approach, I only have a few nits.




Comment at: flang/runtime/environment.cpp:42
+#else
+if (setenv(name, value, 0) == -1) {
+#endif





Comment at: flang/runtime/environment.cpp:77
+#else
+  envp = environ;
+#endif

peixin wrote:
> jpenix-quic wrote:
> > peixin wrote:
> > > What is `environ` used for? Should we try to keep as less extern variable 
> > > as possible in runtime for security.
> > I think `setenv` is only required to make sure that the `environ` pointer 
> > points to the correct copy of the environment variables. So, as long as we 
> > are storing a copy of the environment pointer in `ExecutionEnvironment` and 
> > potentially making changes to the environment via `setenv`, I think we need 
> > to base it off the `environ` pointer after `setenv` has been called rather 
> > than the `envp` from `main`.
> > 
> > That said, I don't think the `envp` variable we are storing is being used 
> > for anything at the moment (reading environment variables was changed to 
> > use `getenv` rather than `envp` in the commit [[ 
> > https://github.com/llvm/llvm-project/commit/824bf908194c9267f1f09065ba8e41d7969006ab
> >  | here]]). If removing the usage of `environ` is preferable, we could 
> > maybe remove the usage of `envp` altogether (in a separate patch)--does 
> > this sound like a good idea or would it be better to just leave in the 
> > `environ` usage for now?
> Thanks for the explanations. The current fix makes sense to me.
I agree that we should remove envp altogether in a follow-up patch. As it 
stands it's just causing confusion.



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

Nit: Shouldn't you also check that valid values (e.g. unknown, swap etc) are 
accepted? 



Comment at: flang/test/Driver/driver-help.f90:27
 ! HELP-NEXT: -fcolor-diagnosticsEnable colors in diagnostics
+! HELP-NEXT: -fconvert=   Set endian conversion of data for 
unformatted files
 ! HELP-NEXT: -fdefault-double-8 Set the default double precision kind to 
an 8 byte wide type

Nit: Why is there an extra blank for these?


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] D128043: [flang][driver] Add support for `-O{0|1|2|3}`

[Diana Picus via Phabricator via cfe-commits]

rovka accepted this revision.
rovka added a comment.

Cool, 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] D128043: [flang][driver] Add support for `-O{0|1|2|3}`

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

I just realized I haven't pestered you enough about testing :) Can you add a 
test that -O4 indeed warns and uses -O3? Also, the summary says this should 
work in both the compilation and the frontend driver, but you're only testing 
with %flang_fc1.




Comment at: flang/test/Driver/default-optimization-pipelines.f90:1
+! Verify that`-O{n}` is indeed taken into account when defining the LLVM 
optimization/middle-end pass pass pipeline.
+




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}`

[Diana Picus via Phabricator via cfe-commits]

rovka added inline comments.



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.

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?



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

This isn't used yet, can we skip it?



Comment at: flang/include/flang/Frontend/FrontendActions.h:202
   bool beginSourceFileAction() override;
+  /// Sets up LLVM's TargetMachine, configures llvmModule accordingly
   void setUpTargetMachine();





Comment at: flang/lib/Frontend/FrontendActions.cpp:617
 /// \param [in] llvmModule LLVM module to lower to assembly/machine-code
 /// \param [out] os Output stream to emit the generated code to
+

Shouldn't this comment go away too?



Comment at: flang/lib/Frontend/FrontendActions.cpp:711
 
+  // Run LLVM's middle-end (i.e. the optimizer)
+  runOptimizationPipeline(*os);





Comment at: flang/lib/Frontend/FrontendActions.cpp:724
   if (action == BackendActionTy::Backend_EmitBC) {
-generateLLVMBCImpl(*tm, *llvmModule, *os);
+// This action has effectively been completed in runOptimizationPipeline
 return;





Comment at: flang/lib/Frontend/FrontendActions.cpp:728
 
+  // Run LLVM's backend and generate either assembly or machine code
   if (action == BackendActionTy::Backend_EmitAssembly ||





Comment at: flang/test/Driver/default-optimization-pipelines.f90:1
+! Verify that`-O{n}` is indeed taken into account when definining the LLVM 
optimization/middle-end pass pass pipeline.
+




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] D126291: [flang][Driver] Update link job on windows

[Diana Picus 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 rG26041e17006c: Update link job for flang on windows (authored 
by rovka).

Changed prior to commit:
  https://reviews.llvm.org/D126291?vs=436722=438271#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126291

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.h
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/lib/Driver/ToolChains/MinGW.cpp
  flang/test/Driver/linker-flags.f90

Index: flang/test/Driver/linker-flags.f90
===
--- flang/test/Driver/linker-flags.f90
+++ flang/test/Driver/linker-flags.f90
@@ -2,18 +2,18 @@
 ! invocation. These libraries are added on top of other standard runtime
 ! libraries that the Clang driver will include.
 
-! NOTE: The additional linker flags tested here are currently only specified for
-! GNU and Darwin. The following line will make sure that this test is skipped on
-! Windows. If you are running this test on a yet another platform and it is
-! failing for you, please either update the following or (preferably) update the
-! linker invocation accordingly.
-! UNSUPPORTED: system-windows
+!-
+! RUN COMMANDS
+!-
+! 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 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's driver library, clangDriver, usually adds 'libcmt' and
+!   'oldnames' on Windows, but they are not needed when compiling
+!   Fortran code and they might bring in additional dependencies.
+!   Make sure they're not added.
+! RUN: %flang -### -flang-experimental-exec -target aarch64-windows-msvc %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,MSVC --implicit-check-not libcmt --implicit-check-not oldnames
 
 !
 ! EXPECTED OUTPUT
@@ -23,9 +23,29 @@
 ! CHECK-SAME:  "-o" "[[object_file:.*\.o]]" {{.*}}Inputs/hello.f90
 
 ! Linker invocation to generate the executable
-! CHECK-LABEL:  "/usr/bin/ld"
-! CHECK-SAME: "[[object_file]]"
-! CHECK-SAME: -lFortran_main
-! CHECK-SAME: -lFortranRuntime
-! CHECK-SAME: -lFortranDecimal
-! CHECK-SAME: -lm
+! GNU-LABEL:  "{{.*}}ld" 
+! GNU-SAME: "[[object_file]]"
+! GNU-SAME: -lFortran_main
+! GNU-SAME: -lFortranRuntime
+! GNU-SAME: -lFortranDecimal
+! GNU-SAME: -lm
+
+! DARWIN-LABEL:  "{{.*}}ld" 
+! DARWIN-SAME: "[[object_file]]"
+! DARWIN-SAME: -lFortran_main
+! DARWIN-SAME: -lFortranRuntime
+! DARWIN-SAME: -lFortranDecimal
+
+! MINGW-LABEL:  "{{.*}}ld" 
+! MINGW-SAME: "[[object_file]]"
+! MINGW-SAME: -lFortran_main
+! MINGW-SAME: -lFortranRuntime
+! MINGW-SAME: -lFortranDecimal
+
+! NOTE: This check should also match if the default linker is lld-link.exe
+! MSVC-LABEL: link.exe
+! MSVC-SAME: Fortran_main.lib
+! MSVC-SAME: FortranRuntime.lib
+! MSVC-SAME: FortranDecimal.lib
+! MSVC-SAME: /subsystem:console
+! MSVC-SAME: "[[object_file]]"
Index: clang/lib/Driver/ToolChains/MinGW.cpp
===
--- clang/lib/Driver/ToolChains/MinGW.cpp
+++ clang/lib/Driver/ToolChains/MinGW.cpp
@@ -218,6 +218,11 @@
 
   AddLinkerInputs(TC, Inputs, Args, CmdArgs, JA);
 
+  if (C.getDriver().IsFlangMode()) {
+addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
+addFortranRuntimeLibs(TC, CmdArgs);
+  }
+
   // TODO: Add profile stuff here
 
   if (TC.ShouldLinkCXXStdlib(Args)) {
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -81,7 +81,7 @@
 Args.MakeArgString(std::string("-out:") + Output.getFilename()));
 
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles) &&
-  !C.getDriver().IsCLMode()) {
+  !C.getDriver().IsCLMode() && !C.getDriver().IsFlangMode()) {
 CmdArgs.push_back("-defaultlib:libcmt");
 CmdArgs.push_back("-defaultlib:oldnames");
   }
@@ -130,6 +130,16 @@
   Args.MakeArgString(std::string("-libpath:") + WindowsSdkLibPath));
   }
 
+  if (C.getDriver().IsFlangMode()) {
+addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
+

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

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

I'm going to go ahead and commit this on Monday if nobody raises any objections 
until then. Thanks again to everyone for all the help & feedback!




Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:773
+  // TODO: Make this work unconditionally once Flang is mature enough.
+  if (!Args.hasArg(options::OPT_flang_experimental_exec))
+return;

mstorsjo wrote:
> Don't you need to have the same check in `addFortranRuntimeLibs` above too? 
> As both of these functions are called without checking the condition in the 
> caller.
I'd rather not, since that would require adding the `ArgList` as an input to 
`addFortranRuntimeLibs`. I don't think it's worth changing the interface just 
for the sake of something temporary. The whole point of checking the flag 
inside `addFortranRuntimeLibraryPath` is to make it really easy to remove the 
check later, without having to update all the callsites.

See also [[ 
https://reviews.llvm.org/D126291#inline-1227536:~:text=this%20flag%20is%20here%20only%20temporarily
 | the previous discussion ]] (I don't know how to get a link to a comment in 
Phab, so I just linked some text inside it).


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

[Diana Picus via Phabricator via cfe-commits]

rovka added inline comments.



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

awarzynski wrote:
> 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 :)
Yep, that's what I was saying :)


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

[Diana Picus via Phabricator via cfe-commits]

rovka updated this revision to Diff 436722.
rovka added a comment.

- Use --implicit-not-check <3


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

https://reviews.llvm.org/D126291

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.h
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/lib/Driver/ToolChains/MinGW.cpp
  flang/test/Driver/linker-flags.f90

Index: flang/test/Driver/linker-flags.f90
===
--- flang/test/Driver/linker-flags.f90
+++ flang/test/Driver/linker-flags.f90
@@ -2,18 +2,17 @@
 ! invocation. These libraries are added on top of other standard runtime
 ! libraries that the Clang driver will include.
 
-! NOTE: The additional linker flags tested here are currently only specified for
-! GNU and Darwin. The following line will make sure that this test is skipped on
-! Windows. If you are running this test on a yet another platform and it is
-! failing for you, please either update the following or (preferably) update the
-! linker invocation accordingly.
-! UNSUPPORTED: system-windows
+!-
+! RUN COMMANDS
+!-
+! 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 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
+!   bring in additional dependencies. Make sure they're not added.
+! RUN: %flang -### -flang-experimental-exec -target aarch64-windows-msvc %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,MSVC --implicit-check-not libcmt --implicit-check-not oldnames
 
 !
 ! EXPECTED OUTPUT
@@ -23,9 +22,29 @@
 ! CHECK-SAME:  "-o" "[[object_file:.*\.o]]" {{.*}}Inputs/hello.f90
 
 ! Linker invocation to generate the executable
-! CHECK-LABEL:  "/usr/bin/ld"
-! CHECK-SAME: "[[object_file]]"
-! CHECK-SAME: -lFortran_main
-! CHECK-SAME: -lFortranRuntime
-! CHECK-SAME: -lFortranDecimal
-! CHECK-SAME: -lm
+! GNU-LABEL:  "{{.*}}ld" 
+! GNU-SAME: "[[object_file]]"
+! GNU-SAME: -lFortran_main
+! GNU-SAME: -lFortranRuntime
+! GNU-SAME: -lFortranDecimal
+! GNU-SAME: -lm
+
+! DARWIN-LABEL:  "{{.*}}ld" 
+! DARWIN-SAME: "[[object_file]]"
+! DARWIN-SAME: -lFortran_main
+! DARWIN-SAME: -lFortranRuntime
+! DARWIN-SAME: -lFortranDecimal
+
+! MINGW-LABEL:  "{{.*}}ld" 
+! MINGW-SAME: "[[object_file]]"
+! MINGW-SAME: -lFortran_main
+! MINGW-SAME: -lFortranRuntime
+! MINGW-SAME: -lFortranDecimal
+
+! NOTE: This check should also match if the default linker is lld-link.exe
+! MSVC-LABEL: link.exe
+! MSVC-SAME: Fortran_main.lib
+! MSVC-SAME: FortranRuntime.lib
+! MSVC-SAME: FortranDecimal.lib
+! MSVC-SAME: /subsystem:console
+! MSVC-SAME: "[[object_file]]"
Index: clang/lib/Driver/ToolChains/MinGW.cpp
===
--- clang/lib/Driver/ToolChains/MinGW.cpp
+++ clang/lib/Driver/ToolChains/MinGW.cpp
@@ -218,6 +218,11 @@
 
   AddLinkerInputs(TC, Inputs, Args, CmdArgs, JA);
 
+  if (C.getDriver().IsFlangMode()) {
+addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
+addFortranRuntimeLibs(TC, CmdArgs);
+  }
+
   // TODO: Add profile stuff here
 
   if (TC.ShouldLinkCXXStdlib(Args)) {
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -81,7 +81,7 @@
 Args.MakeArgString(std::string("-out:") + Output.getFilename()));
 
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles) &&
-  !C.getDriver().IsCLMode()) {
+  !C.getDriver().IsCLMode() && !C.getDriver().IsFlangMode()) {
 CmdArgs.push_back("-defaultlib:libcmt");
 CmdArgs.push_back("-defaultlib:oldnames");
   }
@@ -130,6 +130,16 @@
   Args.MakeArgString(std::string("-libpath:") + WindowsSdkLibPath));
   }
 
+  if (C.getDriver().IsFlangMode()) {
+addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
+addFortranRuntimeLibs(TC, CmdArgs);
+
+// Inform the MSVC linker that we're generating a console application, i.e.
+// one with `main` as the "user-defined" entry point. The `main` function is
+// defined in flang's runtime libraries.
+CmdArgs.push_back("/subsystem:console");
+  }
+
 

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

[Diana Picus via Phabricator via cfe-commits]

rovka added inline comments.



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

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! 


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

[Diana Picus via Phabricator via cfe-commits]

rovka added inline comments.



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

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.


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

[Diana Picus via Phabricator via cfe-commits]

rovka updated this revision to Diff 436687.
rovka added a comment.

Clarified test checks.


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

https://reviews.llvm.org/D126291

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.h
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/lib/Driver/ToolChains/MinGW.cpp
  flang/test/Driver/linker-flags.f90

Index: flang/test/Driver/linker-flags.f90
===
--- flang/test/Driver/linker-flags.f90
+++ flang/test/Driver/linker-flags.f90
@@ -2,18 +2,13 @@
 ! invocation. These libraries are added on top of other standard runtime
 ! libraries that the Clang driver will include.
 
-! NOTE: The additional linker flags tested here are currently only specified for
-! GNU and Darwin. The following line will make sure that this test is skipped on
-! Windows. If you are running this test on a yet another platform and it is
-! failing for you, please either update the following or (preferably) update the
-! linker invocation accordingly.
-! UNSUPPORTED: system-windows
-
-!
-! 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
+!-
+! RUN COMMANDS
+!-
+! 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
 
 !
 ! EXPECTED OUTPUT
@@ -23,9 +18,35 @@
 ! CHECK-SAME:  "-o" "[[object_file:.*\.o]]" {{.*}}Inputs/hello.f90
 
 ! Linker invocation to generate the executable
-! CHECK-LABEL:  "/usr/bin/ld"
-! CHECK-SAME: "[[object_file]]"
-! CHECK-SAME: -lFortran_main
-! CHECK-SAME: -lFortranRuntime
-! CHECK-SAME: -lFortranDecimal
-! CHECK-SAME: -lm
+! GNU-LABEL:  "{{.*}}ld" 
+! GNU-SAME: "[[object_file]]"
+! GNU-SAME: -lFortran_main
+! GNU-SAME: -lFortranRuntime
+! GNU-SAME: -lFortranDecimal
+! GNU-SAME: -lm
+
+! DARWIN-LABEL:  "{{.*}}ld" 
+! DARWIN-SAME: "[[object_file]]"
+! DARWIN-SAME: -lFortran_main
+! DARWIN-SAME: -lFortranRuntime
+! DARWIN-SAME: -lFortranDecimal
+
+! MINGW-LABEL:  "{{.*}}ld" 
+! MINGW-SAME: "[[object_file]]"
+! MINGW-SAME: -lFortran_main
+! MINGW-SAME: -lFortranRuntime
+! MINGW-SAME: -lFortranDecimal
+
+! NOTE: This check should also match if the default linker is lld-link.exe
+! MSVC-LABEL: link.exe
+! These libraries are added by clang, but are not needed when compiling
+! Fortran code, and they might bring in other dependencies.
+! MSVC-NOT: libcmt
+! MSVC-NOT: oldnames
+! MSVC-SAME: Fortran_main.lib
+! MSVC-SAME: FortranRuntime.lib
+! MSVC-SAME: FortranDecimal.lib
+! MSVC-SAME: /subsystem:console
+! MSVC-NOT: libcmt
+! MSVC-NOT: oldnames
+! MSVC-SAME: "[[object_file]]"
Index: clang/lib/Driver/ToolChains/MinGW.cpp
===
--- clang/lib/Driver/ToolChains/MinGW.cpp
+++ clang/lib/Driver/ToolChains/MinGW.cpp
@@ -218,6 +218,11 @@
 
   AddLinkerInputs(TC, Inputs, Args, CmdArgs, JA);
 
+  if (C.getDriver().IsFlangMode()) {
+addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
+addFortranRuntimeLibs(TC, CmdArgs);
+  }
+
   // TODO: Add profile stuff here
 
   if (TC.ShouldLinkCXXStdlib(Args)) {
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -81,7 +81,7 @@
 Args.MakeArgString(std::string("-out:") + Output.getFilename()));
 
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles) &&
-  !C.getDriver().IsCLMode()) {
+  !C.getDriver().IsCLMode() && !C.getDriver().IsFlangMode()) {
 CmdArgs.push_back("-defaultlib:libcmt");
 CmdArgs.push_back("-defaultlib:oldnames");
   }
@@ -130,6 +130,16 @@
   Args.MakeArgString(std::string("-libpath:") + WindowsSdkLibPath));
   }
 
+  if (C.getDriver().IsFlangMode()) {
+addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
+addFortranRuntimeLibs(TC, CmdArgs);
+
+// Inform the MSVC linker that we're generating a console application, i.e.
+// one with `main` as the "user-defined" entry point. The `main` function is
+// defined in flang's runtime libraries.
+CmdArgs.push_back("/subsystem:console");
+  }
+
   // Add the compiler-rt library directories to libpath if 

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

[Diana Picus via Phabricator via cfe-commits]

rovka updated this revision to Diff 436340.
rovka added a comment.

- Stop using --ld-path, since it seems to be ignored on Windows. Instead, just 
match any path that ends in ld (should work with ld, lld, gold). This isn't 
very robust, but I don't have any better ideas (other than actually support 
--ld-path everywhere, not sure how much work that would be).


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

https://reviews.llvm.org/D126291

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.h
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/lib/Driver/ToolChains/MinGW.cpp
  flang/test/Driver/linker-flags.f90

Index: flang/test/Driver/linker-flags.f90
===
--- flang/test/Driver/linker-flags.f90
+++ flang/test/Driver/linker-flags.f90
@@ -2,18 +2,13 @@
 ! invocation. These libraries are added on top of other standard runtime
 ! libraries that the Clang driver will include.
 
-! NOTE: The additional linker flags tested here are currently only specified for
-! GNU and Darwin. The following line will make sure that this test is skipped on
-! Windows. If you are running this test on a yet another platform and it is
-! failing for you, please either update the following or (preferably) update the
-! linker invocation accordingly.
-! UNSUPPORTED: system-windows
-
-!
-! 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
+!-
+! RUN COMMANDS
+!-
+! RUN: %flang -### -flang-experimental-exec -target ppc64le-linux-gnu %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,GNU-WITHLM
+! RUN: %flang -### -flang-experimental-exec -target aarch64-apple-darwin %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,GNU-WITHOUTLM
+! RUN: %flang -### -flang-experimental-exec -target x86_64-windows-gnu %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,GNU-WITHOUTLM
+! RUN: %flang -### -flang-experimental-exec -target aarch64-windows-msvc %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,MSVC
 
 !
 ! EXPECTED OUTPUT
@@ -22,10 +17,31 @@
 ! CHECK-LABEL: {{.*}} "-emit-obj"
 ! CHECK-SAME:  "-o" "[[object_file:.*\.o]]" {{.*}}Inputs/hello.f90
 
-! Linker invocation to generate the executable
-! CHECK-LABEL:  "/usr/bin/ld"
-! CHECK-SAME: "[[object_file]]"
-! CHECK-SAME: -lFortran_main
-! CHECK-SAME: -lFortranRuntime
-! CHECK-SAME: -lFortranDecimal
-! CHECK-SAME: -lm
+! Linker invocation to generate the executable - GNU version
+! GNU-WITHLM-LABEL:  "{{.*}}ld" 
+! GNU-WITHLM-SAME: "[[object_file]]"
+! GNU-WITHLM-SAME: -lFortran_main
+! GNU-WITHLM-SAME: -lFortranRuntime
+! GNU-WITHLM-SAME: -lFortranDecimal
+! GNU-WITHLM-SAME: -lm
+
+! GNU-WITHOUTLM-LABEL:  "{{.*}}ld" 
+! GNU-WITHOUTLM-SAME: "[[object_file]]"
+! GNU-WITHOUTLM-SAME: -lFortran_main
+! GNU-WITHOUTLM-SAME: -lFortranRuntime
+! GNU-WITHOUTLM-SAME: -lFortranDecimal
+
+! Linker invocation to generate the executable - MSVC version
+! NOTE: This check should also match if the default linker is lld-link.exe
+! MSVC-LABEL: link.exe
+! These libraries are added by clang, but are not needed when compiling
+! Fortran code, and they might bring in other dependencies.
+! MSVC-NOT: libcmt
+! MSVC-NOT: oldnames
+! MSVC-SAME: Fortran_main.lib
+! MSVC-SAME: FortranRuntime.lib
+! MSVC-SAME: FortranDecimal.lib
+! MSVC-SAME: /subsystem:console
+! MSVC-NOT: libcmt
+! MSVC-NOT: oldnames
+! MSVC-SAME: "[[object_file]]"
Index: clang/lib/Driver/ToolChains/MinGW.cpp
===
--- clang/lib/Driver/ToolChains/MinGW.cpp
+++ clang/lib/Driver/ToolChains/MinGW.cpp
@@ -218,6 +218,11 @@
 
   AddLinkerInputs(TC, Inputs, Args, CmdArgs, JA);
 
+  if (C.getDriver().IsFlangMode()) {
+addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
+addFortranRuntimeLibs(TC, CmdArgs);
+  }
+
   // TODO: Add profile stuff here
 
   if (TC.ShouldLinkCXXStdlib(Args)) {
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -81,7 +81,7 @@
 Args.MakeArgString(std::string("-out:") + Output.getFilename()));
 
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles) &&
-  !C.getDriver().IsCLMode()) {
+  !C.getDriver().IsCLMode() && !C.getDriver().IsFlangMode()) {
 CmdArgs.push_back("-defaultlib:libcmt");
 CmdArgs.push_back("-defaultlib:oldnames");
   }
@@ -130,6 +130,16 @@
   Args.MakeArgString(std::string("-libpath:") + WindowsSdkLibPath));
   }
 
+  if (C.getDriver().IsFlangMode()) {
+addFortranRuntimeLibraryPath(TC, Args, 

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

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

I'm guessing the Windows precommit is failing because --ld-path is ignored on 
Windows, even if we use a Linux target? I have a fix that works on my Windows 
machine, coming right up.




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

awarzynski wrote:
> 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?
It doesn't say that the previous match has to be with the same prefix, and it 
seems to work :)
But since it looks funny to at least one person, I'll update it to use the same 
prefix.



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

awarzynski wrote:
> Is it worth adding a comment to explain why to single these out?
I guess it won't hurt 


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

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

In D126291#3577159 , @mstorsjo wrote:

> In D126291#3577133 , @rovka wrote:
>
>> I had the same idea about switching the tests to using target triples 
>> instead of having separate files for it, but initially I had some issues 
>> getting that to work properly. When specifying a triple, we need to provide 
>> an architecture. Leaving the triple as `unkown-linux-gnu` or just 
>> `linux-gnu` gives us an error along the lines of `flang-new: error: unknown 
>> target triple 'unknown-unknown-linux-gnu', please use -triple or -arch`. 
>> OTOH, hardcoding an architecture like x86 or aarch64 fails if we're not 
>> building that specific backend.
>
> If you actually execute code generation, then yes, it fails if that specific 
> arch isn't enabled. But for general compiler driver level tests, which just 
> print out the command the would have executed (when running with `-###`), it 
> should work without the actual code generation target being available. This 
> is at least how it's done for clang's corresponding tests, most files in 
> `clang/test/Driver` have hardcoded arch triples, without any REQUIRES lines 
> or other exclusions.

Oops, you're right, I was testing with a wrong x86 triple >.< I'll simplify the 
patch then.


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

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

In D126291#3572796 , @mmuetzel wrote:

> In D126291#3572777 , @rovka wrote:
>
>> Moved the check for `-flang-experimental-exec` into 
>> addFortranRuntimeLibraryPath, so it affects all the toolchains. @awarzynski 
>> does this look like a good idea?
>
> If moving that check to inside that function is ok, should the same check be 
> added to `addFortranRuntimeLibs`, too?
> Edit: And also retain that condition for any flags that are added in the 
> respective part of the toolchain files that don't use any of these two 
> functions?

I don't think we need to add it to the other function since we'll get an error 
anyway if the linker can't find the libraries. In any case, the right way to 
handle this is probably to error out in the driver before trying to compose a 
link job when `-flang-experimental-exec` is not specified, rather than rely on 
a specific linker error. But that should probably be a different patch. 
@awarzynski wdyt?


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

[Diana Picus via Phabricator via cfe-commits]

rovka updated this revision to Diff 436301.
rovka added a comment.

Missed a spot (removing the namespace in MinGW.cpp)


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

https://reviews.llvm.org/D126291

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.h
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/lib/Driver/ToolChains/MinGW.cpp
  flang/test/Driver/linker-flags.f90
  flang/test/lit.cfg.py

Index: flang/test/lit.cfg.py
===
--- flang/test/lit.cfg.py
+++ flang/test/lit.cfg.py
@@ -45,6 +45,17 @@
 for arch in config.targets_to_build.split():
 config.available_features.add(arch.lower() + '-registered-target')
 
+# Some driver tests need to use a supported and registered architecture as part
+# of the target triple, so we define a substitution for it. Note that it doesn't
+# matter which of the supported targets is used as long as it is enabled in
+# LLVM_TARGETS_TO_BUILD.
+if 'AArch64' in config.targets:
+config.substitutions.append(('%flangarch%', 'aarch64'))
+elif 'PowerPC' in config.targets:
+config.substitutions.append(('%flangarch%', 'ppc64le'))
+elif 'X86' in config.targets:
+config.substitutions.append(('%flangarch%', 'x86_64'))
+
 # excludes: A list of directories to exclude from the testsuite. The 'Inputs'
 # subdirectories contain auxiliary inputs for various tests in their parent
 # directories.
Index: flang/test/Driver/linker-flags.f90
===
--- flang/test/Driver/linker-flags.f90
+++ flang/test/Driver/linker-flags.f90
@@ -2,18 +2,15 @@
 ! invocation. These libraries are added on top of other standard runtime
 ! libraries that the Clang driver will include.
 
-! NOTE: The additional linker flags tested here are currently only specified for
-! GNU and Darwin. The following line will make sure that this test is skipped on
-! Windows. If you are running this test on a yet another platform and it is
-! failing for you, please either update the following or (preferably) update the
-! linker invocation accordingly.
-! UNSUPPORTED: system-windows
-
-!
-! RUN COMMAND
-!
+!-
+! RUN COMMANDS
+!-
 ! 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 that `--ld-path` is ignored on some systems.
+! RUN: %flang -### -flang-experimental-exec -target %flangarch%-linux-gnu --ld-path=/usr/bin/ld %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,GNU,WITHLM
+! RUN: %flang -### -flang-experimental-exec -target %flangarch%-apple-darwin --ld-path=/usr/bin/ld %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,GNU
+! RUN: %flang -### -flang-experimental-exec -target %flangarch%-windows-gnu --ld-path=/usr/bin/ld %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,GNU
+! RUN: %flang -### -flang-experimental-exec -target %flangarch%-windows-msvc %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,MSVC
 
 !
 ! EXPECTED OUTPUT
@@ -22,10 +19,21 @@
 ! CHECK-LABEL: {{.*}} "-emit-obj"
 ! CHECK-SAME:  "-o" "[[object_file:.*\.o]]" {{.*}}Inputs/hello.f90
 
-! Linker invocation to generate the executable
-! CHECK-LABEL:  "/usr/bin/ld"
-! CHECK-SAME: "[[object_file]]"
-! CHECK-SAME: -lFortran_main
-! CHECK-SAME: -lFortranRuntime
-! CHECK-SAME: -lFortranDecimal
-! CHECK-SAME: -lm
+! Linker invocation to generate the executable - GNU version
+! GNU-LABEL:  "/usr/bin/ld"
+! GNU-SAME: "[[object_file]]"
+! GNU-SAME: -lFortran_main
+! GNU-SAME: -lFortranRuntime
+! GNU-SAME: -lFortranDecimal
+! WITHLM-SAME: -lm
+
+! Linker invocation to generate the executable - MSVC version
+! NOTE: This check should also match if the default linker is lld-link.exe
+! MSVC-LABEL: link.exe
+! MSVC-NOT: libcmt
+! MSVC-NOT: oldnames
+! MSVC-SAME: Fortran_main.lib
+! MSVC-SAME: FortranRuntime.lib
+! MSVC-SAME: FortranDecimal.lib
+! MSVC-SAME: /subsystem:console
+! MSVC-SAME: "[[object_file]]"
Index: clang/lib/Driver/ToolChains/MinGW.cpp
===
--- clang/lib/Driver/ToolChains/MinGW.cpp
+++ clang/lib/Driver/ToolChains/MinGW.cpp
@@ -218,6 +218,11 @@
 
   AddLinkerInputs(TC, Inputs, Args, CmdArgs, JA);
 
+  if (C.getDriver().IsFlangMode()) {
+addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
+addFortranRuntimeLibs(TC, CmdArgs);
+  }
+
   // TODO: Add profile stuff here
 
   if (TC.ShouldLinkCXXStdlib(Args)) {
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -81,7 +81,7 @@
 

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

[Diana Picus via Phabricator via cfe-commits]

rovka updated this revision to Diff 436300.
rovka added a comment.
Herald added a subscriber: ormris.

- Switch to the new style of testing (using triples rather than separate files)
- Fix oversight in Darwin toolchain file

I hope I've addressed all the comments and nothing slipped past during rebase. 
Thanks again for all the contributions :)


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

https://reviews.llvm.org/D126291

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.h
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/lib/Driver/ToolChains/MinGW.cpp
  flang/test/Driver/linker-flags.f90
  flang/test/lit.cfg.py

Index: flang/test/lit.cfg.py
===
--- flang/test/lit.cfg.py
+++ flang/test/lit.cfg.py
@@ -45,6 +45,17 @@
 for arch in config.targets_to_build.split():
 config.available_features.add(arch.lower() + '-registered-target')
 
+# Some driver tests need to use a supported and registered architecture as part
+# of the target triple, so we define a substitution for it. Note that it doesn't
+# matter which of the supported targets is used as long as it is enabled in
+# LLVM_TARGETS_TO_BUILD.
+if 'AArch64' in config.targets:
+config.substitutions.append(('%flangarch%', 'aarch64'))
+elif 'PowerPC' in config.targets:
+config.substitutions.append(('%flangarch%', 'ppc64le'))
+elif 'X86' in config.targets:
+config.substitutions.append(('%flangarch%', 'x86_64'))
+
 # excludes: A list of directories to exclude from the testsuite. The 'Inputs'
 # subdirectories contain auxiliary inputs for various tests in their parent
 # directories.
Index: flang/test/Driver/linker-flags.f90
===
--- flang/test/Driver/linker-flags.f90
+++ flang/test/Driver/linker-flags.f90
@@ -2,18 +2,15 @@
 ! invocation. These libraries are added on top of other standard runtime
 ! libraries that the Clang driver will include.
 
-! NOTE: The additional linker flags tested here are currently only specified for
-! GNU and Darwin. The following line will make sure that this test is skipped on
-! Windows. If you are running this test on a yet another platform and it is
-! failing for you, please either update the following or (preferably) update the
-! linker invocation accordingly.
-! UNSUPPORTED: system-windows
-
-!
-! RUN COMMAND
-!
+!-
+! RUN COMMANDS
+!-
 ! 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 that `--ld-path` is ignored on some systems.
+! RUN: %flang -### -flang-experimental-exec -target %flangarch%-linux-gnu --ld-path=/usr/bin/ld %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,GNU,WITHLM
+! RUN: %flang -### -flang-experimental-exec -target %flangarch%-apple-darwin --ld-path=/usr/bin/ld %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,GNU
+! RUN: %flang -### -flang-experimental-exec -target %flangarch%-windows-gnu --ld-path=/usr/bin/ld %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,GNU
+! RUN: %flang -### -flang-experimental-exec -target %flangarch%-windows-msvc %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,MSVC
 
 !
 ! EXPECTED OUTPUT
@@ -22,10 +19,21 @@
 ! CHECK-LABEL: {{.*}} "-emit-obj"
 ! CHECK-SAME:  "-o" "[[object_file:.*\.o]]" {{.*}}Inputs/hello.f90
 
-! Linker invocation to generate the executable
-! CHECK-LABEL:  "/usr/bin/ld"
-! CHECK-SAME: "[[object_file]]"
-! CHECK-SAME: -lFortran_main
-! CHECK-SAME: -lFortranRuntime
-! CHECK-SAME: -lFortranDecimal
-! CHECK-SAME: -lm
+! Linker invocation to generate the executable - GNU version
+! GNU-LABEL:  "/usr/bin/ld"
+! GNU-SAME: "[[object_file]]"
+! GNU-SAME: -lFortran_main
+! GNU-SAME: -lFortranRuntime
+! GNU-SAME: -lFortranDecimal
+! WITHLM-SAME: -lm
+
+! Linker invocation to generate the executable - MSVC version
+! NOTE: This check should also match if the default linker is lld-link.exe
+! MSVC-LABEL: link.exe
+! MSVC-NOT: libcmt
+! MSVC-NOT: oldnames
+! MSVC-SAME: Fortran_main.lib
+! MSVC-SAME: FortranRuntime.lib
+! MSVC-SAME: FortranDecimal.lib
+! MSVC-SAME: /subsystem:console
+! MSVC-SAME: "[[object_file]]"
Index: clang/lib/Driver/ToolChains/MinGW.cpp
===
--- clang/lib/Driver/ToolChains/MinGW.cpp
+++ clang/lib/Driver/ToolChains/MinGW.cpp
@@ -218,6 +218,11 @@
 
   AddLinkerInputs(TC, Inputs, Args, CmdArgs, JA);
 
+  if (C.getDriver().IsFlangMode()) {
+tools::addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
+tools::addFortranRuntimeLibs(TC, CmdArgs);
+  }
+
   // TODO: Add profile stuff here
 
   if (TC.ShouldLinkCXXStdlib(Args)) {
Index: 

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

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

I had the same idea about switching the tests to using target triples instead 
of having separate files for it, but initially I had some issues getting that 
to work properly. When specifying a triple, we need to provide an architecture. 
Leaving the triple as `unkown-linux-gnu` or just `linux-gnu` gives us an error 
along the lines of `flang-new: error: unknown target triple 
'unknown-unknown-linux-gnu', please use -triple or -arch`. OTOH, hardcoding an 
architecture like x86 or aarch64 fails if we're not building that specific 
backend. We could do that and make the test REQUIRE the architecture that we're 
hardcoding, but this isn't really an architecture-specific test. So what I've 
finally done instead is to check for flang supported architectures and add a 
lit substitution for the first one that we find (be it aarch64, powerpc or x86) 
and use that in the test. We'll still get an error if someone tries to build 
the test without enabling any of these targets, but I think that's a good 
thing, since then people can decide either to add their architecture to lit or 
just, you know, not build flang on platforms where it isn't supported :) Patch 
incoming.


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

[Diana Picus via Phabricator via cfe-commits]

rovka updated this revision to Diff 435842.
rovka added a comment.

Moved the check for `-flang-experimental-exec` into 
addFortranRuntimeLibraryPath, so it affects all the toolchains. @awarzynski 
does this look like a good idea?


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

https://reviews.llvm.org/D126291

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.h
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/lib/Driver/ToolChains/MinGW.cpp
  flang/test/Driver/linker-flags-windows.f90
  flang/test/Driver/linker-flags.f90

Index: flang/test/Driver/linker-flags.f90
===
--- flang/test/Driver/linker-flags.f90
+++ flang/test/Driver/linker-flags.f90
@@ -5,8 +5,8 @@
 ! NOTE: The additional linker flags tested here are currently only specified for
 ! GNU and Darwin. The following line will make sure that this test is skipped on
 ! Windows. If you are running this test on a yet another platform and it is
-! failing for you, please either update the following or (preferably) update the
-! linker invocation accordingly.
+! failing for you, please either update the following or (preferably)
+! create a similar test for your platform.
 ! UNSUPPORTED: system-windows
 
 !
Index: flang/test/Driver/linker-flags-windows.f90
===
--- /dev/null
+++ flang/test/Driver/linker-flags-windows.f90
@@ -0,0 +1,25 @@
+! Verify that the Fortran runtime libraries are present in the linker
+! invocation. These libraries are added on top of other standard runtime
+! libraries that the Clang driver will include.
+
+! NOTE: The additional linker flags tested here are currently specified in
+! clang/lib/Driver/Toolchains/MSVC.cpp.
+! REQUIRES: windows-msvc
+
+! RUN: %flang -### %S/Inputs/hello.f90 2>&1 | FileCheck %s
+
+! Compiler invocation to generate the object file
+! CHECK-LABEL: {{.*}} "-emit-obj"
+! CHECK-SAME:  "-o" "[[object_file:.*]]" {{.*}}Inputs/hello.f90
+
+! Linker invocation to generate the executable
+! NOTE: This check should also match if the default linker is lld-link.exe
+! CHECK-LABEL: link.exe
+! CHECK-NOT: libcmt
+! CHECK-NOT: oldnames
+! CHECK-SAME: Fortran_main.lib
+! CHECK-SAME: FortranRuntime.lib
+! CHECK-SAME: FortranDecimal.lib
+! CHECK-SAME: /subsystem:console
+! CHECK-SAME: "[[object_file]]"
+
Index: clang/lib/Driver/ToolChains/MinGW.cpp
===
--- clang/lib/Driver/ToolChains/MinGW.cpp
+++ clang/lib/Driver/ToolChains/MinGW.cpp
@@ -218,6 +218,11 @@
 
   AddLinkerInputs(TC, Inputs, Args, CmdArgs, JA);
 
+  if (C.getDriver().IsFlangMode()) {
+tools::addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
+tools::addFortranRuntimeLibs(TC, CmdArgs);
+  }
+
   // TODO: Add profile stuff here
 
   if (TC.ShouldLinkCXXStdlib(Args)) {
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -81,7 +81,7 @@
 Args.MakeArgString(std::string("-out:") + Output.getFilename()));
 
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles) &&
-  !C.getDriver().IsCLMode()) {
+  !C.getDriver().IsCLMode() && !C.getDriver().IsFlangMode()) {
 CmdArgs.push_back("-defaultlib:libcmt");
 CmdArgs.push_back("-defaultlib:oldnames");
   }
@@ -130,6 +130,16 @@
   Args.MakeArgString(std::string("-libpath:") + WindowsSdkLibPath));
   }
 
+  if (C.getDriver().IsFlangMode()) {
+tools::addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
+tools::addFortranRuntimeLibs(TC, CmdArgs);
+
+// Inform the MSVC linker that we're generating a console application, i.e.
+// one with `main` as the "user-defined" entry point. The `main` function is
+// defined in flang's runtime libraries.
+CmdArgs.push_back("/subsystem:console");
+  }
+
   // Add the compiler-rt library directories to libpath if they exist to help
   // the linker find the various sanitizer, builtin, and profiling runtimes.
   for (const auto  : TC.getLibraryPaths()) {
Index: clang/lib/Driver/ToolChains/Gnu.cpp
===
--- clang/lib/Driver/ToolChains/Gnu.cpp
+++ clang/lib/Driver/ToolChains/Gnu.cpp
@@ -593,13 +593,9 @@
   // to generate executables. As Fortran runtime depends on the C runtime,
   // these dependencies need to be listed before the C runtime below (i.e.
   // AddRuntTimeLibs).
-  //
-  // NOTE: Generating executables by Flang is considered an "experimental"
-  // feature and hence this is guarded with a command line option.
-  // TODO: Make this work unconditionally once Flang is mature enough.
-  if (D.IsFlangMode() && Args.hasArg(options::OPT_flang_experimental_exec)) {
+  if 

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

[Diana Picus via Phabricator via cfe-commits]

rovka added inline comments.



Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:133
 
+  if (C.getDriver().IsFlangMode()) {
+tools::addFortranRuntimeLibraryPath(TC, Args, CmdArgs);

mmuetzel wrote:
> The GNU toolchain has this conditional on 
> `Args.hasArg(options::OPT_flang_experimental_exec)`.
> Should this require that command line flag on MSVC, too?
> 
> Same for the MinGW toolchain file.
Right, that would be nice :D 


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

[Diana Picus via Phabricator via cfe-commits]

rovka updated this revision to Diff 435150.
rovka edited the summary of this revision.
rovka added a comment.

- Fixed test
- Unconditionally added the subsystem arg
- Incorporated the MinGW toolchain changes (Thanks again @mmuetzel, I'm adding 
you as co-author)

@awarzynski I agree that the other patch (replacing ) should be 
committed separately. @mmuetzel are you going to upload it to Phab? I gave it a 
try on my machine and I can't compile the runtime with it, but I'll try to fix 
it on my end and we can continue the discussion in a new revision.


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

https://reviews.llvm.org/D126291

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.h
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/lib/Driver/ToolChains/MinGW.cpp
  flang/test/Driver/linker-flags-windows.f90
  flang/test/Driver/linker-flags.f90

Index: flang/test/Driver/linker-flags.f90
===
--- flang/test/Driver/linker-flags.f90
+++ flang/test/Driver/linker-flags.f90
@@ -5,8 +5,8 @@
 ! NOTE: The additional linker flags tested here are currently only specified for
 ! GNU and Darwin. The following line will make sure that this test is skipped on
 ! Windows. If you are running this test on a yet another platform and it is
-! failing for you, please either update the following or (preferably) update the
-! linker invocation accordingly.
+! failing for you, please either update the following or (preferably)
+! create a similar test for your platform.
 ! UNSUPPORTED: system-windows
 
 !
Index: flang/test/Driver/linker-flags-windows.f90
===
--- /dev/null
+++ flang/test/Driver/linker-flags-windows.f90
@@ -0,0 +1,25 @@
+! Verify that the Fortran runtime libraries are present in the linker
+! invocation. These libraries are added on top of other standard runtime
+! libraries that the Clang driver will include.
+
+! NOTE: The additional linker flags tested here are currently specified in
+! clang/lib/Driver/Toolchains/MSVC.cpp.
+! REQUIRES: windows-msvc
+
+! RUN: %flang -### %S/Inputs/hello.f90 2>&1 | FileCheck %s
+
+! Compiler invocation to generate the object file
+! CHECK-LABEL: {{.*}} "-emit-obj"
+! CHECK-SAME:  "-o" "[[object_file:.*]]" {{.*}}Inputs/hello.f90
+
+! Linker invocation to generate the executable
+! NOTE: This check should also match if the default linker is lld-link.exe
+! CHECK-LABEL: link.exe
+! CHECK-NOT: libcmt
+! CHECK-NOT: oldnames
+! CHECK-SAME: Fortran_main.lib
+! CHECK-SAME: FortranRuntime.lib
+! CHECK-SAME: FortranDecimal.lib
+! CHECK-SAME: /subsystem:console
+! CHECK-SAME: "[[object_file]]"
+
Index: clang/lib/Driver/ToolChains/MinGW.cpp
===
--- clang/lib/Driver/ToolChains/MinGW.cpp
+++ clang/lib/Driver/ToolChains/MinGW.cpp
@@ -218,6 +218,11 @@
 
   AddLinkerInputs(TC, Inputs, Args, CmdArgs, JA);
 
+  if (C.getDriver().IsFlangMode()) {
+tools::addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
+tools::addFortranRuntimeLibs(TC, CmdArgs);
+  }
+
   // TODO: Add profile stuff here
 
   if (TC.ShouldLinkCXXStdlib(Args)) {
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -81,7 +81,7 @@
 Args.MakeArgString(std::string("-out:") + Output.getFilename()));
 
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles) &&
-  !C.getDriver().IsCLMode()) {
+  !C.getDriver().IsCLMode() && !C.getDriver().IsFlangMode()) {
 CmdArgs.push_back("-defaultlib:libcmt");
 CmdArgs.push_back("-defaultlib:oldnames");
   }
@@ -130,6 +130,16 @@
   Args.MakeArgString(std::string("-libpath:") + WindowsSdkLibPath));
   }
 
+  if (C.getDriver().IsFlangMode()) {
+tools::addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
+tools::addFortranRuntimeLibs(TC, CmdArgs);
+
+// Inform the MSVC linker that we're generating a console application, i.e.
+// one with `main` as the "user-defined" entry point. The `main` function is
+// defined in flang's runtime libraries.
+CmdArgs.push_back("/subsystem:console");
+  }
+
   // Add the compiler-rt library directories to libpath if they exist to help
   // the linker find the various sanitizer, builtin, and profiling runtimes.
   for (const auto  : TC.getLibraryPaths()) {
Index: clang/lib/Driver/ToolChains/Gnu.cpp
===
--- clang/lib/Driver/ToolChains/Gnu.cpp
+++ clang/lib/Driver/ToolChains/Gnu.cpp
@@ -599,7 +599,7 @@
   // TODO: Make this work unconditionally once Flang is mature enough.
   if (D.IsFlangMode() && Args.hasArg(options::OPT_flang_experimental_exec)) {
 

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

[Diana Picus via Phabricator via cfe-commits]

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

LGTM, thanks.




Comment at: flang/lib/Frontend/CompilerInvocation.cpp:268
+ // pre-processed inputs.
+.Case("f95", Language::Fortran)
+.Case("f95-cpp-input", Language::Fortran)

ekieri wrote:
> Is there a reason to change from "f90" to "f95"? In my understanding, "f90" 
> is more idiomatic for free-form fortran of any standard >= 90.
At least for `gfortran`, `f90` doesn't seem to be supported, only `f77`, 
`f77-cpp-input`, `f95`,  `f95-cpp-input` are.
https://raw.githubusercontent.com/gcc-mirror/gcc/master/gcc/doc/invoke.texi#:~:text=f77%20%20f77%2Dcpp%2Dinput%20f95%20%20f95%2Dcpp%2Dinput

Note that these are not file extensions, but values for the `-x` option.



Comment at: flang/test/Driver/parse-ir-error.f95:16
+!
+! CHECK: error: expected integer
+! CHECK: error: Could not parse IR

Nit: I think checking just the "Could not parse IR" message should be enough.


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] D126291: [flang][Driver] Update link job on windows

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

@mmuetzel Thanks for looking into this! I think since you're passing 
`-DCLANG_DEFAULT_RTLIB=compiler-rt`, you might indeed need to build compiler-rt 
(or at least the builtins part of it).

FYI, I'll be out of office until Wednesday, but I'll definitely check all the 
comments here when I get back. Thanks again for all the help.


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

[Diana Picus via Phabricator via cfe-commits]

rovka updated this revision to Diff 433988.
rovka added a comment.

- Update for MinGW.
- Add `/subsystem:console` to help `link.exe` understand what's going on.

Thanks for all the comments. I don't have a MinGW environment readily available 
for testing. @mmuetzel Could you test this? Alternatively, do you know whether 
we have any buildbots that would test this?


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

https://reviews.llvm.org/D126291

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.h
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/MSVC.cpp
  flang/test/Driver/linker-flags-windows.f90
  flang/test/Driver/linker-flags.f90

Index: flang/test/Driver/linker-flags.f90
===
--- flang/test/Driver/linker-flags.f90
+++ flang/test/Driver/linker-flags.f90
@@ -5,9 +5,9 @@
 ! NOTE: The additional linker flags tested here are currently only specified for
 ! GNU and Darwin. The following line will make sure that this test is skipped on
 ! Windows. If you are running this test on a yet another platform and it is
-! failing for you, please either update the following or (preferably) update the
-! linker invocation accordingly.
-! UNSUPPORTED: system-windows
+! failing for you, please either update the following or (preferably)
+! create a similar test for your platform.
+! UNSUPPORTED: windows-msvc
 
 !
 ! RUN COMMAND
Index: flang/test/Driver/linker-flags-windows.f90
===
--- /dev/null
+++ flang/test/Driver/linker-flags-windows.f90
@@ -0,0 +1,25 @@
+! Verify that the Fortran runtime libraries are present in the linker
+! invocation. These libraries are added on top of other standard runtime
+! libraries that the Clang driver will include.
+
+! NOTE: The additional linker flags tested here are currently specified in
+! clang/lib/Driver/Toolchains/MSVC.cpp.
+! REQUIRES: windows-msvc
+
+! RUN: %flang -### %S/Inputs/hello.f90 2>&1 | FileCheck %s
+
+! Compiler invocation to generate the object file
+! CHECK-LABEL: {{.*}} "-emit-obj"
+! CHECK-SAME:  "-o" "[[object_file:.*]]" {{.*}}Inputs/hello.f90
+
+! Linker invocation to generate the executable
+! NOTE: This check should also match if the default linker is lld-link.exe
+! CHECK-LABEL: link.exe
+! CHECK-NOT: libcmt
+! CHECK-NOT: oldnames
+! CHECK-SAME: Fortran_main.lib
+! CHECK-SAME: FortranRuntime.lib
+! CHECK-SAME: FortranDecimal.lib
+! CHECK-SAME: /subsystem:console
+! CHECK-SAME: "[[object_file]]"
+
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -81,7 +81,7 @@
 Args.MakeArgString(std::string("-out:") + Output.getFilename()));
 
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles) &&
-  !C.getDriver().IsCLMode()) {
+  !C.getDriver().IsCLMode() && !C.getDriver().IsFlangMode()) {
 CmdArgs.push_back("-defaultlib:libcmt");
 CmdArgs.push_back("-defaultlib:oldnames");
   }
@@ -130,6 +130,17 @@
   Args.MakeArgString(std::string("-libpath:") + WindowsSdkLibPath));
   }
 
+  if (C.getDriver().IsFlangMode()) {
+tools::addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
+tools::addFortranRuntimeLibs(TC, CmdArgs);
+
+// Inform the MSVC linker that we're generating a console application, i.e.
+// one with `main` as the "user-defined" entry point. The `main` function is
+// defined in flang's runtime libraries.
+if (TC.getTriple().isKnownWindowsMSVCEnvironment())
+  CmdArgs.push_back("/subsystem:console");
+  }
+
   // Add the compiler-rt library directories to libpath if they exist to help
   // the linker find the various sanitizer, builtin, and profiling runtimes.
   for (const auto  : TC.getLibraryPaths()) {
Index: clang/lib/Driver/ToolChains/Gnu.cpp
===
--- clang/lib/Driver/ToolChains/Gnu.cpp
+++ clang/lib/Driver/ToolChains/Gnu.cpp
@@ -599,7 +599,7 @@
   // TODO: Make this work unconditionally once Flang is mature enough.
   if (D.IsFlangMode() && Args.hasArg(options::OPT_flang_experimental_exec)) {
 addFortranRuntimeLibraryPath(ToolChain, Args, CmdArgs);
-addFortranRuntimeLibs(CmdArgs);
+addFortranRuntimeLibs(ToolChain, CmdArgs);
 CmdArgs.push_back("-lm");
   }
 
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -644,7 +644,7 @@
   if (getToolChain().getDriver().IsFlangMode() &&
   Args.hasArg(options::OPT_flang_experimental_exec)) {
 addFortranRuntimeLibraryPath(getToolChain(), Args, CmdArgs);
-addFortranRuntimeLibs(CmdArgs);
+

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

[Diana Picus via Phabricator via cfe-commits]

rovka added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:753
+  llvm::opt::ArgStringList ) {
+  if (TC.getTriple().isOSWindows()) {
+CmdArgs.push_back("Fortran_main.lib");

mmuetzel wrote:
> Are those correct for MinGW? Should this be checking for 
> `TC.getTriple().isKnownWindowsMSVCEnvironment()` instead?
Right, I hadn't considered MinGW. Thanks for pointing it out, I'll update the 
patch.


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

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

In D126291#3550563 , @Meinersbur 
wrote:

> While the test passes now, I still get `LINK : fatal error LNK1561: entry 
> point must be defined` when trying to actually link. Isn't it expected to not 
> work yet?
>
> Linking with `lld-link.exe` does work. objdump says there is a `_QQmain` 
> symbol, why does lld consider this to be the entry point?

It doesn't - we're linking in a `main` as part of the `Fortran_main.lib`, which 
then calls `_QQmain`.

I don't really know why `link.exe` doesn't work. According to the docs 
,
 it should find `main` and choose the subsystem on its own. The only hunch I 
have is that maybe it doesn't work because `main` is in a library as opposed to 
one of the object files, but I'm still going through the docs trying to figure 
this out. I don't have a lot of experience with `link.exe` so I might be 
missing something obvious.


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

[Diana Picus via Phabricator via cfe-commits]

rovka updated this revision to Diff 433318.
rovka added a comment.

Updated comment in `linker-flags.f90` test


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

https://reviews.llvm.org/D126291

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.h
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/MSVC.cpp
  flang/test/Driver/linker-flags-windows.f90
  flang/test/Driver/linker-flags.f90

Index: flang/test/Driver/linker-flags.f90
===
--- flang/test/Driver/linker-flags.f90
+++ flang/test/Driver/linker-flags.f90
@@ -5,8 +5,8 @@
 ! NOTE: The additional linker flags tested here are currently only specified for
 ! GNU and Darwin. The following line will make sure that this test is skipped on
 ! Windows. If you are running this test on a yet another platform and it is
-! failing for you, please either update the following or (preferably) update the
-! linker invocation accordingly.
+! failing for you, please either update the following or (preferably)
+! create a similar test for your platform.
 ! UNSUPPORTED: system-windows
 
 !
Index: flang/test/Driver/linker-flags-windows.f90
===
--- /dev/null
+++ flang/test/Driver/linker-flags-windows.f90
@@ -0,0 +1,24 @@
+! Verify that the Fortran runtime libraries are present in the linker
+! invocation. These libraries are added on top of other standard runtime
+! libraries that the Clang driver will include.
+
+! NOTE: The additional linker flags tested here are currently specified in
+! clang/lib/Driver/Toolchains/MSVC.cpp.
+! REQUIRES: system-windows
+
+! RUN: %flang -### %S/Inputs/hello.f90 2>&1 | FileCheck %s
+
+! Compiler invocation to generate the object file
+! CHECK-LABEL: {{.*}} "-emit-obj"
+! CHECK-SAME:  "-o" "[[object_file:.*]]" {{.*}}Inputs/hello.f90
+
+! Linker invocation to generate the executable
+! NOTE: This check should also match if the default linker is lld-link.exe
+! CHECK-LABEL: link.exe
+! CHECK-NOT: libcmt
+! CHECK-NOT: oldnames
+! CHECK-SAME: Fortran_main.lib
+! CHECK-SAME: FortranRuntime.lib
+! CHECK-SAME: FortranDecimal.lib
+! CHECK-SAME: "[[object_file]]"
+
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -81,7 +81,7 @@
 Args.MakeArgString(std::string("-out:") + Output.getFilename()));
 
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles) &&
-  !C.getDriver().IsCLMode()) {
+  !C.getDriver().IsCLMode() && !C.getDriver().IsFlangMode()) {
 CmdArgs.push_back("-defaultlib:libcmt");
 CmdArgs.push_back("-defaultlib:oldnames");
   }
@@ -130,6 +130,11 @@
   Args.MakeArgString(std::string("-libpath:") + WindowsSdkLibPath));
   }
 
+  if (C.getDriver().IsFlangMode()) {
+tools::addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
+tools::addFortranRuntimeLibs(TC, CmdArgs);
+  }
+
   // Add the compiler-rt library directories to libpath if they exist to help
   // the linker find the various sanitizer, builtin, and profiling runtimes.
   for (const auto  : TC.getLibraryPaths()) {
Index: clang/lib/Driver/ToolChains/Gnu.cpp
===
--- clang/lib/Driver/ToolChains/Gnu.cpp
+++ clang/lib/Driver/ToolChains/Gnu.cpp
@@ -599,7 +599,7 @@
   // TODO: Make this work unconditionally once Flang is mature enough.
   if (D.IsFlangMode() && Args.hasArg(options::OPT_flang_experimental_exec)) {
 addFortranRuntimeLibraryPath(ToolChain, Args, CmdArgs);
-addFortranRuntimeLibs(CmdArgs);
+addFortranRuntimeLibs(ToolChain, CmdArgs);
 CmdArgs.push_back("-lm");
   }
 
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -644,7 +644,7 @@
   if (getToolChain().getDriver().IsFlangMode() &&
   Args.hasArg(options::OPT_flang_experimental_exec)) {
 addFortranRuntimeLibraryPath(getToolChain(), Args, CmdArgs);
-addFortranRuntimeLibs(CmdArgs);
+addFortranRuntimeLibs(getToolChain(), CmdArgs);
   }
 
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nodefaultlibs))
Index: clang/lib/Driver/ToolChains/CommonArgs.h
===
--- clang/lib/Driver/ToolChains/CommonArgs.h
+++ clang/lib/Driver/ToolChains/CommonArgs.h
@@ -121,7 +121,8 @@
   bool IsOffloadingHost = false, bool GompNeedsRT = false);
 
 /// Adds Fortran runtime libraries to \p CmdArgs.
-void addFortranRuntimeLibs(llvm::opt::ArgStringList );
+void addFortranRuntimeLibs(const ToolChain ,
+   llvm::opt::ArgStringList );
 
 /// Adds the path for 

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

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

In D126291#3547142 , @awarzynski 
wrote:

> In D126291#3547023 , @rovka wrote:
>
>> With this patch in place, we still need to add `-Xlinker -subsystem:console' 
>> in order to compile a simple 'Hello World' to run from the console.
>
> Is this behavior consistent with "clang.exe"? Also, any clue why is this 
> needed? Some documentation here 
> .
>  I've scanned it and am no wiser :/

Yep, it is consistent with clang. I tried running both `clang-cl.exe` and 
`clang.exe` on a `hello.cpp` and in both cases I get a civilized error from lld 
saying "lld-link: error: subsystem must be defined". 
I've read those docs too and I think the main takeaway is that it sets the 
entry point for the program. There's a nice table 

 that suggests that passing in `/subsystem:console` ultimately leads to `main` 
being called, which is definitely what we want, since that's what we're linking 
in via `Fortran_main`.
Now, it could be the case that for Fortran we could hardcode the subsystem 
since people probably (?!) don't write kernel mode drivers or boot applications 
in Fortran, but I'm not so sure about the GUI stuff (i.e. the windows 
subsystem). Since I don't know enough about these things, I'd rather leave 
users the option to use whatever subsystem they like (and hope things won't 
break just because we're linking in `Fortran_main.lib`). I'd love to hear more 
opinions though.


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

[Diana Picus via Phabricator via cfe-commits]

rovka added inline comments.



Comment at: flang/test/Driver/flang-linker-flags-windows.f90:16
+! Linker invocation to generate the executable
+! CHECK-LABEL:  lld-link.exe
+! CHECK-NOT: libcmt

Meinersbur wrote:
> This is failing for me. Instead of `lld-link.exe`, it is using `link.exe` 
> (the Microsoft linker).
> 
> Without the `-###` flag, the link command is failing:
> ```
> flang-new version 15.0.0 (C:/Users/meinersbur/src/llvm-project/clang 
> 09fdf5f6f5e70ecb7d95cdbb98442c998a55ce23)
> Target: x86_64-pc-windows-msvc
> Thread model: posix
> InstalledDir: c:\users\meinersbur\build\llvm-project\release\bin
>  "c:\\users\\meinersbur\\build\\llvm-project\\release\\bin\\flang-new" -fc1 
> -triple x86_64-pc-windows-msvc19.32.31329 -emit-obj -o 
> "C:\\Users\\MEINER~1\\AppData\\Local\\Temp\\hello-ae4145.o" 
> "C:\\Users\\meinersbur\\src\\llvm-project\\flang\\test\\Driver/Inputs/hello.f90"
>  "C:\\Program Files\\Microsoft Visual 
> Studio\\2022\\Community\\VC\\Tools\\MSVC\\14.32.31326\\bin\\Hostx64\\x64\\link.exe"
>  -out:a.exe 
> "-libpath:c:\\users\\meinersbur\\build\\llvm-project\\release\\lib" 
> Fortran_main.lib FortranRuntime.lib FortranDecimal.lib -nologo 
> "C:\\Users\\MEINER~1\\AppData\\Local\\Temp\\hello-ae4145.o"
> LINK : fatal error LNK1561: entry point must be defined
> flang-new: error: linker command failed with exit code 1561 (use -v to see 
> invocation)
> ```
This might be because you're not passing any subsystem. Can you try with 
`-Xlinker -subsystem:console`?


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

[Diana Picus via Phabricator via cfe-commits]

rovka updated this revision to Diff 433054.
rovka added a comment.

Updated test to check for link.exe instead of lld-link.exe. This doesn't look 
great but it works for both lld-link.exe and the default link.exe. I tried to 
use --ld-path=various/paths instead, but it seems to be ignored on Windows (I 
even get a warning about "argument unused during compilation"). If anyone has 
any ideas about how to make the test more robust, please let me know.


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

https://reviews.llvm.org/D126291

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.h
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/MSVC.cpp
  flang/test/Driver/linker-flags-windows.f90

Index: flang/test/Driver/linker-flags-windows.f90
===
--- /dev/null
+++ flang/test/Driver/linker-flags-windows.f90
@@ -0,0 +1,24 @@
+! Verify that the Fortran runtime libraries are present in the linker
+! invocation. These libraries are added on top of other standard runtime
+! libraries that the Clang driver will include.
+
+! NOTE: The additional linker flags tested here are currently specified in
+! clang/lib/Driver/Toolchains/MSVC.cpp.
+! REQUIRES: system-windows
+
+! RUN: %flang -### %S/Inputs/hello.f90 2>&1 | FileCheck %s
+
+! Compiler invocation to generate the object file
+! CHECK-LABEL: {{.*}} "-emit-obj"
+! CHECK-SAME:  "-o" "[[object_file:.*]]" {{.*}}Inputs/hello.f90
+
+! Linker invocation to generate the executable
+! NOTE: This check should also match if the default linker is lld-link.exe
+! CHECK-LABEL: link.exe
+! CHECK-NOT: libcmt
+! CHECK-NOT: oldnames
+! CHECK-SAME: Fortran_main.lib
+! CHECK-SAME: FortranRuntime.lib
+! CHECK-SAME: FortranDecimal.lib
+! CHECK-SAME: "[[object_file]]"
+
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -81,7 +81,7 @@
 Args.MakeArgString(std::string("-out:") + Output.getFilename()));
 
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles) &&
-  !C.getDriver().IsCLMode()) {
+  !C.getDriver().IsCLMode() && !C.getDriver().IsFlangMode()) {
 CmdArgs.push_back("-defaultlib:libcmt");
 CmdArgs.push_back("-defaultlib:oldnames");
   }
@@ -130,6 +130,11 @@
   Args.MakeArgString(std::string("-libpath:") + WindowsSdkLibPath));
   }
 
+  if (C.getDriver().IsFlangMode()) {
+tools::addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
+tools::addFortranRuntimeLibs(TC, CmdArgs);
+  }
+
   // Add the compiler-rt library directories to libpath if they exist to help
   // the linker find the various sanitizer, builtin, and profiling runtimes.
   for (const auto  : TC.getLibraryPaths()) {
Index: clang/lib/Driver/ToolChains/Gnu.cpp
===
--- clang/lib/Driver/ToolChains/Gnu.cpp
+++ clang/lib/Driver/ToolChains/Gnu.cpp
@@ -599,7 +599,7 @@
   // TODO: Make this work unconditionally once Flang is mature enough.
   if (D.IsFlangMode() && Args.hasArg(options::OPT_flang_experimental_exec)) {
 addFortranRuntimeLibraryPath(ToolChain, Args, CmdArgs);
-addFortranRuntimeLibs(CmdArgs);
+addFortranRuntimeLibs(ToolChain, CmdArgs);
 CmdArgs.push_back("-lm");
   }
 
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -644,7 +644,7 @@
   if (getToolChain().getDriver().IsFlangMode() &&
   Args.hasArg(options::OPT_flang_experimental_exec)) {
 addFortranRuntimeLibraryPath(getToolChain(), Args, CmdArgs);
-addFortranRuntimeLibs(CmdArgs);
+addFortranRuntimeLibs(getToolChain(), CmdArgs);
   }
 
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nodefaultlibs))
Index: clang/lib/Driver/ToolChains/CommonArgs.h
===
--- clang/lib/Driver/ToolChains/CommonArgs.h
+++ clang/lib/Driver/ToolChains/CommonArgs.h
@@ -121,7 +121,8 @@
   bool IsOffloadingHost = false, bool GompNeedsRT = false);
 
 /// Adds Fortran runtime libraries to \p CmdArgs.
-void addFortranRuntimeLibs(llvm::opt::ArgStringList );
+void addFortranRuntimeLibs(const ToolChain ,
+   llvm::opt::ArgStringList );
 
 /// Adds the path for the Fortran runtime libraries to \p CmdArgs.
 void addFortranRuntimeLibraryPath(const ToolChain ,
Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -748,10 +748,17 @@
   return true;
 }
 
-void tools::addFortranRuntimeLibs(llvm::opt::ArgStringList ) {

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

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

In D126291#3536718 , 
@kiranchandramohan wrote:

> @rovka in case you missed this, the test is failing in windows.

Yeah, thanks, I was out of office a bit so I couldn't get to it sooner. I'll 
give it a try without lld.


Repository:
  rG LLVM Github Monorepo

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

[Diana Picus via Phabricator via cfe-commits]

rovka created this revision.
rovka added reviewers: awarzynski, kiranchandramohan.
rovka added a project: Flang.
Herald added a subscriber: jdoerfert.
Herald added a reviewer: sscalpone.
Herald added a project: All.
rovka requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.

When linking a Fortran program, we need to add the runtime libraries to
the command line. This is exactly what we do for Linux/Darwin, but the
interface is slightly different (e.g. -libpath instead of -L).

We also remove oldnames and libcmt, since they're not
needed at the moment and they bring in more dependencies.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126291

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.h
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/MSVC.cpp
  flang/test/Driver/flang-linker-flags-windows.f90

Index: flang/test/Driver/flang-linker-flags-windows.f90
===
--- /dev/null
+++ flang/test/Driver/flang-linker-flags-windows.f90
@@ -0,0 +1,23 @@
+! Verify that the Fortran runtime libraries are present in the linker
+! invocation. These libraries are added on top of other standard runtime
+! libraries that the Clang driver will include.
+
+! NOTE: The additional linker flags tested here are currently specified in
+! clang/lib/Driver/Toolchains/MSVC.cpp.
+! REQUIRES: system-windows
+
+! RUN: %flang -### %S/Inputs/hello.f90 2>&1 | FileCheck %s
+
+! Compiler invocation to generate the object file
+! CHECK-LABEL: {{.*}} "-emit-obj"
+! CHECK-SAME:  "-o" "[[object_file:.*]]" {{.*}}Inputs/hello.f90
+
+! Linker invocation to generate the executable
+! CHECK-LABEL:  lld-link.exe
+! CHECK-NOT: libcmt
+! CHECK-NOT: oldnames
+! CHECK-SAME: Fortran_main.lib
+! CHECK-SAME: FortranRuntime.lib
+! CHECK-SAME: FortranDecimal.lib
+! CHECK-SAME: "[[object_file]]"
+
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -81,7 +81,7 @@
 Args.MakeArgString(std::string("-out:") + Output.getFilename()));
 
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles) &&
-  !C.getDriver().IsCLMode()) {
+  !C.getDriver().IsCLMode() && !C.getDriver().IsFlangMode()) {
 CmdArgs.push_back("-defaultlib:libcmt");
 CmdArgs.push_back("-defaultlib:oldnames");
   }
@@ -130,6 +130,11 @@
   Args.MakeArgString(std::string("-libpath:") + WindowsSdkLibPath));
   }
 
+  if (C.getDriver().IsFlangMode()) {
+tools::addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
+tools::addFortranRuntimeLibs(TC, CmdArgs);
+  }
+
   // Add the compiler-rt library directories to libpath if they exist to help
   // the linker find the various sanitizer, builtin, and profiling runtimes.
   for (const auto  : TC.getLibraryPaths()) {
Index: clang/lib/Driver/ToolChains/Gnu.cpp
===
--- clang/lib/Driver/ToolChains/Gnu.cpp
+++ clang/lib/Driver/ToolChains/Gnu.cpp
@@ -599,7 +599,7 @@
   // TODO: Make this work unconditionally once Flang is mature enough.
   if (D.IsFlangMode() && Args.hasArg(options::OPT_flang_experimental_exec)) {
 addFortranRuntimeLibraryPath(ToolChain, Args, CmdArgs);
-addFortranRuntimeLibs(CmdArgs);
+addFortranRuntimeLibs(ToolChain, CmdArgs);
 CmdArgs.push_back("-lm");
   }
 
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -644,7 +644,7 @@
   if (getToolChain().getDriver().IsFlangMode() &&
   Args.hasArg(options::OPT_flang_experimental_exec)) {
 addFortranRuntimeLibraryPath(getToolChain(), Args, CmdArgs);
-addFortranRuntimeLibs(CmdArgs);
+addFortranRuntimeLibs(getToolChain(), CmdArgs);
   }
 
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nodefaultlibs))
Index: clang/lib/Driver/ToolChains/CommonArgs.h
===
--- clang/lib/Driver/ToolChains/CommonArgs.h
+++ clang/lib/Driver/ToolChains/CommonArgs.h
@@ -121,7 +121,8 @@
   bool IsOffloadingHost = false, bool GompNeedsRT = false);
 
 /// Adds Fortran runtime libraries to \p CmdArgs.
-void addFortranRuntimeLibs(llvm::opt::ArgStringList );
+void addFortranRuntimeLibs(const ToolChain ,
+   llvm::opt::ArgStringList );
 
 /// Adds the path for the Fortran runtime libraries to \p CmdArgs.
 void addFortranRuntimeLibraryPath(const ToolChain ,
Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ 

[PATCH] D125957: [flang][driver] Make driver accept `-module-dir`

[Diana Picus via Phabricator via cfe-commits]

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

Seems legit, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125957

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

[PATCH] D125628: [flang][driver] Add support for generating executables on MacOSX/Darwin

[Diana Picus via Phabricator via cfe-commits]

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

LGTM, I'll try to send one for Windows this week.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125628

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

[PATCH] D124669: [flang][driver] Add support for -save-temps

[Diana Picus via Phabricator via cfe-commits]

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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124669

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

[PATCH] D124669: [flang][driver] Add support for -save-temps

[Diana Picus via Phabricator via cfe-commits]

rovka added inline comments.



Comment at: flang/test/Driver/fno-integrated-as.f90:6
+!--
+! Verify that there _is_ a seperate line with an assembler invocation
+! RUN: %flang -c -fno-integrated-as %s -### 2>&1 | FileCheck %s





Comment at: flang/test/Driver/fno-integrated-as.f90:10
+! CHECK-SAME: "-o" "[[assembly_file:.*]].s"
+! CHECK-NEXT: "-o" "fno-integrated-as.o" "[[assembly_file:.*]].s"
+





Comment at: flang/test/Driver/fno-integrated-as.f90:18
+! DEFAULT-LABEL: "-fc1"
+! DEFAULT-SAME: "-o" "fno-integrated-as.o" "{{.*}}fno-integrated-as.f90"

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



Comment at: flang/test/Driver/save-temps.f90:1
+! Tests for the `-save-temps` flag. As `flang` does not implement `-fc1as` 
(i.e. a driver for the intergrated assembly), we need to
+! use `-fno-integrated-as` here.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124669

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

[PATCH] D124669: [flang][driver] Add support for -save-temps

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124669

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

[PATCH] D124669: [flang][driver] Add support for -save-temps

[Diana Picus via Phabricator via cfe-commits]

rovka added inline comments.



Comment at: clang/include/clang/Driver/Options.td:4131
 def : Flag<["-"], "no-integrated-as">, Alias,
-  Flags<[CC1Option, NoXarchOption]>;
+  Flags<[CC1Option,FlangOption,NoXarchOption]>;
 

awarzynski wrote:
> unterumarmung wrote:
> > Why not to add `FC1Option` here and elsewhere like it's done for 
> > `CC1Option`?
> I'm not 100% sure what `-fno-integrated-as` controls in Clang's frontend 
> driver, `clang -cc1`. I'm guessing that it might related to using/not-using 
> LLVM's MCAsmParser. Perhaps for inline assembly?
> 
> In Flang, I'm only focusing on `-save-temps` for which I need to make sure 
> that we don't try to call `flang-new -fc1as` or something similar (it does 
> not exist). Instead, `-save-temps` will have to rely on an external assembler.
> 
> So, we basically don't require -`fno-integrated-as` in `flang-new -fc1` just 
> yet (that's what `FC1Option` is for - marking an option as available in the 
> frontend driver).
I'm not 100% sure either (haven't looked at the code), but my understanding of 
`-fno-integrated-as` is that it forces clang to call the system assembler as 
opposed to using the LLVM libraries to generate machine code directly. Since 
flang never uses the system assembler, I would say the integrated assembler is 
always on in flang. So I'm not convinced it makes sense to add this flag to the 
driver, unless we're actually adding a 
`-fc1as`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124669

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

[PATCH] D124667: [flang][driver] Add support for consuming LLVM IR/BC files

[Diana Picus via Phabricator via cfe-commits]

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

A few nits, but otherwise LGTM.




Comment at: flang/lib/Frontend/FrontendActions.cpp:86
+
+  // ... otherwise, generate an MLIR module from the input Fortran source
   bool res = RunPrescan() && RunParse() && RunSemanticChecks();

Nit: Should we assert that the language is Language::Fortran at this point?



Comment at: flang/test/Driver/emit-asm-from-llvm-bc.ll:2
+; Verify that the driver can consume LLVM BC files. The expected assembly is
+; fairly (tested on AArch64 an X86_64), but we may need to tweak when testing
+; on other platforms. Note that that the actual output doesn't matter as long





Comment at: flang/test/Driver/emit-asm-from-llvm-bc.ll:3
+; fairly (tested on AArch64 an X86_64), but we may need to tweak when testing
+; on other platforms. Note that that the actual output doesn't matter as long
+; as it's in Assembly format.





Comment at: flang/test/Driver/emit-asm-from-llvm-bc.ll:9
+;-
+; RUN: rm -f %t.bc
+; RUN: %flang_fc1 -emit-llvm-bc %s -o %t.bc

Have you tried %basename_t.bc instead? You might be able to skip all the `rm`s 
then...



Comment at: flang/test/Driver/emit-asm-from-llvm.ll:1
+; Verify that the driver can consume LLVM IR files. The expected assembly is ;
+; fairly generic (verified on AArch64 an X86_64), but we may need to tweak when





Comment at: flang/test/Driver/emit-asm-from-llvm.ll:2
+; Verify that the driver can consume LLVM IR files. The expected assembly is ;
+; fairly generic (verified on AArch64 an X86_64), but we may need to tweak when
+; testing on other platforms. Note that that the actual output doesn't matter





Comment at: flang/test/Driver/emit-asm-from-llvm.ll:3
+; fairly generic (verified on AArch64 an X86_64), but we may need to tweak when
+; testing on other platforms. Note that that the actual output doesn't matter
+; as long as it's in Assembly format.





Comment at: flang/test/Driver/override-triple.ll:20
+; For the triple to be overridden by the driver, it needs to be different to 
the host triple.
+; Use a random string to guaranteed that.
+target triple = "invalid-triple"




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124667

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

[PATCH] D123967: Disable update_cc_test_checks.py tests in stand-alone builds

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

I just have a couple of nits, I'll leave it to the clang devs to properly 
review this.




Comment at: clang/test/CMakeLists.txt:17
   LLVM_WITH_Z3
+  CLANG_BUILT_STANDALONE
   )

Nit: These seem to be sorted alphabetically.



Comment at: clang/test/utils/update_cc_test_checks/lit.local.cfg:17
+# These tests are only relevant to developers working with the
+# update_cc_test_checks.py tool they don't don't provide any coverage
+# for any of the clang source code.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123967

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

[PATCH] D123297: [flang][driver] Add support for -mmlir

[Diana Picus via Phabricator via cfe-commits]

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

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123297

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

[PATCH] D123297: [flang][driver] Add support for -mmlir

[Diana Picus via Phabricator via cfe-commits]

rovka added inline comments.



Comment at: flang/test/Driver/mllvm_vs_mmlir.f90:17
+! MLLVM: flang (LLVM option parsing) [options]
+! MLLVM: --print-ir-after-all
+! MLLVM-NOT: --mlir-{{.*}}

awarzynski wrote:
> awarzynski wrote:
> > rriddle wrote:
> > > awarzynski wrote:
> > > > awarzynski wrote:
> > > > > rovka wrote:
> > > > > > rovka wrote:
> > > > > > > Is this the most llvm-ish option we have? I'm concerned that MLIR 
> > > > > > > might also decide to add an option that sounds like this (and for 
> > > > > > > sure -print-ir-before-all is mentioned in the [[ 
> > > > > > > https://mlir.llvm.org/getting_started/Debugging/ |  MLIR docs ]]).
> > > > > > > Is this the most llvm-ish option we have? I'm concerned that MLIR 
> > > > > > > might also decide to add an option that sounds like this (and for 
> > > > > > > sure -print-ir-before-all is mentioned in the [[ 
> > > > > > > https://mlir.llvm.org/getting_started/Debugging/ |  MLIR docs ]]).
> > > > > > 
> > > > > > Right, I think that might be why the premerge tests are failing...
> > > > > > Is this the most llvm-ish option we have? 
> > > > > 
> > > > > Sadly, most LLVM options have rather generic names that could at some 
> > > > > point be added in MLIR. Perhaps you'll spot something more suitable:
> > > > > 
> > > > > ```lang=bash
> > > > > USAGE: flang (LLVM option parsing) [options]
> > > > > 
> > > > > OPTIONS:
> > > > > 
> > > > > Color Options:
> > > > > 
> > > > >   --color   - Use colors in 
> > > > > output (default=autodetect)
> > > > > 
> > > > > General options:
> > > > > 
> > > > >   --aarch64-neon-syntax= - Choose style of 
> > > > > NEON code to emit from AArch64 backend:
> > > > > =generic-   Emit generic 
> > > > > NEON assembly
> > > > > =apple  -   Emit 
> > > > > Apple-style NEON assembly
> > > > >   --aarch64-use-aa  - Enable the use 
> > > > > of AA during codegen.
> > > > >   --abort-on-max-devirt-iterations-reached  - Abort when the 
> > > > > max iterations for devirtualization CGSCC repeat pass is reached
> > > > >   --allow-ginsert-as-artifact   - Allow G_INSERT 
> > > > > to be considered an artifact. Hack around AMDGPU test infinite loops.
> > > > >   --always-execute-loop-body- force the body 
> > > > > of a loop to execute at least once
> > > > >   --array-constructor-initial-buffer-size=- set the 
> > > > > incremental array construction buffer size (default=32)
> > > > >   --cfg-hide-cold-paths=- Hide blocks 
> > > > > with relative frequency below the given value
> > > > >   --cfg-hide-deoptimize-paths   -
> > > > >   --cfg-hide-unreachable-paths  -
> > > > >   --cost-kind=   - Target cost kind
> > > > > =throughput -   Reciprocal 
> > > > > throughput
> > > > > =latency-   Instruction 
> > > > > latency
> > > > > =code-size  -   Code size
> > > > > =size-latency   -   Code size and 
> > > > > latency
> > > > >   --debugify-level=  - Kind of debug 
> > > > > info to add
> > > > > =locations  -   Locations only
> > > > > =location+variables -   Locations and 
> > > > > Variables
> > > > >   --debugify-quiet  - Suppress 
> > > > > verbose debugify output
> > > > >   --default-kinds= - string to set 
> > > > > default kind values
> > > > >   --disable-i2p-p2i-opt - Disables 
> > > > > inttoptr/ptrtoint roundtrip optimization
> > > > >   --dot-cfg-mssa= - file name for 
> > > > > generated dot file
> > > > >   --enable-cse-in-irtranslator  - Should enable 
> > > > > CSE in irtranslator
> > > > >   --enable-cse-in-legalizer - Should enable 
> > > > > CSE in Legalizer
> > > > >   --enable-gvn-memdep   -
> > > > >   --enable-load-in-loop-pre -
> > > > >   --enable-load-pre -
> > > > >   --enable-loop-simplifycfg-term-folding-
> > > > >   --enable-name-compression - Enable 
> > > > > name/filename string compression
> > > > >   --enable-split-backedge-in-load-pre   -
> > > > >   --experimental-debug-variable-locations   - Use 
> > > > > experimental new value-tracking variable locations
> > > > >   --fdebug-dump-pre-fir - dump the 
> > > > > Pre-FIR tree prior to FIR generation
> > > > >   --fs-profile-debug-bw-threshold=- Only 

[PATCH] D123297: [flang][driver] Add support for -mmlir

[Diana Picus via Phabricator via cfe-commits]

rovka added inline comments.



Comment at: flang/test/Driver/mllvm_vs_mmlir.f90:17
+! MLLVM: flang (LLVM option parsing) [options]
+! MLLVM: --print-ir-after-all
+! MLLVM-NOT: --mlir-{{.*}}

rovka wrote:
> Is this the most llvm-ish option we have? I'm concerned that MLIR might also 
> decide to add an option that sounds like this (and for sure 
> -print-ir-before-all is mentioned in the [[ 
> https://mlir.llvm.org/getting_started/Debugging/ |  MLIR docs ]]).
> Is this the most llvm-ish option we have? I'm concerned that MLIR might also 
> decide to add an option that sounds like this (and for sure 
> -print-ir-before-all is mentioned in the [[ 
> https://mlir.llvm.org/getting_started/Debugging/ |  MLIR docs ]]).

Right, I think that might be why the premerge tests are failing...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123297

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

[PATCH] D123297: [flang][driver] Add support for -mmlir

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

I don't know the command line library that well, so I have this curiosity: what 
happens if LLVM and MLIR have 2 different options with the same name? Do we get 
a compile time error? Or is there a risk that someone might -mllvm -XYZ and it 
would end up in MLIR's XYZ option instead, because we're processing the MLIR 
options after the LLVM ones?




Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:15
+#include "mlir/IR/MLIRContext.h"
+#include "mlir/Pass/PassManager.h"
 #include "flang/Frontend/CompilerInstance.h"

Nit: Should these come after the llvm/ headers? (So it's alphabetical except 
for flang, which may go first)



Comment at: flang/test/Driver/mllvm_vs_mmlir.f90:17
+! MLLVM: flang (LLVM option parsing) [options]
+! MLLVM: --print-ir-after-all
+! MLLVM-NOT: --mlir-{{.*}}

Is this the most llvm-ish option we have? I'm concerned that MLIR might also 
decide to add an option that sounds like this (and for sure 
-print-ir-before-all is mentioned in the [[ 
https://mlir.llvm.org/getting_started/Debugging/ |  MLIR docs ]]).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123297

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

[PATCH] D122008: [flang][driver] Add support for generating executables

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

Just for the record, I'd like to speak in favor of option 2 - merge the patch 
now, but put the functionality behind a flag. This sounds like a good 
compromise to me. I think if we choose an inconspicuous name like 
-fexperimentail-test-driver-integration-with-cmake (or something even more 
obfuscated) we'll be off most users' radar, while at the same time making 
progress with cmake. We should probably first confirm that it would be 
acceptable for the cmake developers to go ahead with something like that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122008

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

[PATCH] D123211: [flang][driver] Add support for generating LLVM bytecode files

[Diana Picus via Phabricator via cfe-commits]

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

In D123211#3433059 , @awarzynski 
wrote:

> Thanks for taking a look Diana!
>
>> Why not use the BackendAction for this?
>
> This action only requires the middle-end in LLVM :) The `BackendAction` will 
> use the backend, but that's not needed here.

Is there actually a significant difference, besides the naming (which is easy 
to change)? AFAICT the BackendAction isn't initializing anything 
backend-related except very close to where it's using it, so that can happen 
inside a switch/if branch.

>> There seems to be a lot of shared code, up until the point where you create 
>> and use the pass manager
>
> This is the bit that's shared:
>
>   CompilerInstance  = this->instance();
>   // Generate an LLVM module if it's not already present (it will already be
>   // present if the input file is an LLVM IR/BC file).
>   if (!llvmModule)
> GenerateLLVMIR();
>   
>   // Create and configure `Target`
>   std::string error;
>   std::string theTriple = llvmModule->getTargetTriple();
>   const llvm::Target *theTarget =
>   llvm::TargetRegistry::lookupTarget(theTriple, error);
>   assert(theTarget && "Failed to create Target");
>   
>   // Create and configure `TargetMachine`
>   std::unique_ptr TM;
>   
>   TM.reset(theTarget->createTargetMachine(theTriple, /*CPU=*/"",
>   /*Features=*/"", llvm::TargetOptions(), llvm::None));
>   assert(TM && "Failed to create TargetMachine");
>   llvmModule->setDataLayout(TM->createDataLayout());
>
> I wouldn't say it's "a lot", but probably enough for a dedicated method. I've 
> been conservative with regard to sharing code as I anticipate things to 
> change in the future (I expect `-O{0|1|2|3|s|z}` to complicate the logic a 
> bit).

I was thinking also about the code for generating the output file, which can be 
folded into the switch from BackendAction. If you consider that too, it becomes 
a very large percentage of EmitLLVMBitcodeAction::ExecuteAction that can be 
shared.

>> (and in the future, when the backend switches to the new pass manager, there 
>> will be even more shared code).
>
> I'm not sure about the timelines for this. And even then, the logic might be 
> quite different (again, middle-end vs back-end).

Ok. IMO the template method pattern would work well here (or less formally, 
just a simple switch to the same effect), but I can understand if you think 
it's premature to go that route. 
No other objections from my side, thanks :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123211

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

[PATCH] D123211: [flang][driver] Add support for generating LLVM bytecode files

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

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




Comment at: flang/lib/Frontend/FrontendActions.cpp:483
+
+  llvm::ModulePassManager MPM;
+  llvm::ModuleAnalysisManager MAM;

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



Comment at: flang/test/CMakeLists.txt:58
   bbc
+  llvm-dis
   llvm-objdump

Were you going to add a test that uses this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123211

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

[PATCH] D122008: [flang][driver] Add support for generating executables

[Diana Picus via Phabricator via cfe-commits]

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

LGTM :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122008

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

[PATCH] D120568: [flang][driver] Add support for -S and implement -c/-emit-obj

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

I don't have any other comments, I'll let Eric take it from here :)




Comment at: flang/lib/Frontend/FrontendActions.cpp:509
+  TargetRegistry::lookupTarget(theTriple, error);
+  assert(theTarget && "Failed to create Target");
+

awarzynski wrote:
> rovka wrote:
> > Shouldn't this be a diagnostic? People could be trying to compile a random 
> > IR file with a triple that's not registered.
> Good point! However, I'm struggling to trigger this assert ATM and I don't 
> want to issue a diagnostic if I can't test it.  This will change once 
> `flang-new` can consume LLVM IR files (soon). In the meantime, let me add a 
> `TODO`. Hopefully we won't forget to elevate this to a proper diagnostic when 
> the time is right :)
TODO is fine for now, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120568

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

[PATCH] D120568: [flang][driver] Add support for -S and implement -c/-emit-obj

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

This passes on Windows on Arm if you add a forward declaration for  `class 
PassInstrumentation` here 
.
 This is necessary because otherwise clang picks up the PassInstrumentation 
from MLIR instead of the PassInstrumentation defined lower in the file.

For reference, this is the error I was seeing before:

  
C:\Work\diana.picus\llvm-project\llvm\include\llvm/IR/PassInstrumentation.h(145,16):
 warning: unqualified friend declaration referring to type outside of the 
nearest enclosing namespace is a Microsoft extension; add a nested name 
specifier [-Wmicrosoft-unqualified-friend]
friend class PassInstrumentation;
 ^
 ::mlir::
  
C:\Work\diana.picus\llvm-project\llvm\include\llvm/IR/PassInstrumentation.h(292,33):
 error: 'AnalysesClearedCallbacks' is a private member of 
'llvm::PassInstrumentationCallbacks'
for (auto  : Callbacks->AnalysesClearedCallbacks)
  ^
  
C:\Work\diana.picus\llvm-project\llvm\include\llvm/IR/PassInstrumentation.h(173,7):
 note: declared private here
AnalysesClearedCallbacks;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120568

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

[PATCH] D120568: [flang][driver] Add support for -S and implement -c/-emit-obj

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

I'm having trouble building this on Windows on Arm. I'm still investigating if 
it's a problem with the patch or just growing pains on the WoA side :) I'll try 
to get to the bottom of it as soon as I can.




Comment at: flang/lib/Frontend/FrontendActions.cpp:509
+  TargetRegistry::lookupTarget(theTriple, error);
+  assert(theTarget && "Failed to create Target");
+

Shouldn't this be a diagnostic? People could be trying to compile a random IR 
file with a triple that's not registered.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120568

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

[PATCH] D120246: [flang][driver] Add support for `--target`/`--triple`

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

Nit: Should we also have a test for print-effective-triple?

Otherwise LGTM, although I'm not sure that -emit-llvm is necessarily something 
we'd want flang users to be exposed to.




Comment at: flang/include/flang/Frontend/TargetOptions.h:20
+/// Options for controlling the target. Currently this is just a placeholder.
+/// In the future we will use this for options to specify various target
+/// options that will affect the generated code e.g.:




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120246

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

[PATCH] D119918: [CMake] Rename TARGET_TRIPLE to LLVM_TARGET_TRIPLE

[Diana Picus via Phabricator via cfe-commits]

rovka accepted this revision.
rovka added a comment.

I'm happy if the buildbots are happy :D thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119918

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

[PATCH] D119918: [CMake] Rename TARGET_TRIPLE to LLVM_TARGET_TRIPLE

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

Is this going to break for everyone that still passes TARGET_TRIPLE to cmake? 
If so, we should probably have a period where we support both.

In particular, this might break some of our buildbots 

 who explicitly set TARGET_TRIPLE. Could you send a patch to zorg cleaning that 
up (and maybe wait for that to go through before removing TARGET_TRIPLE 
completely)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119918

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

[PATCH] D119012: [flang][driver] Add support for the `-emit-llvm` option

[Diana Picus via Phabricator via cfe-commits]

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

LGTM, thanks! I just have a minor nit, feel free to ignore it.




Comment at: flang/lib/Frontend/FrontendActions.cpp:471
+
+  if (!ci.IsOutputStreamNull()) {
+llvmModule->print(

Nit: Can this be folded into the above if? (I.e. just write to os if you 
managed to create it, and add an else branch for the other case)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119012

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

[PATCH] D119012: [flang][driver] Add support for the `-emit-llvm` option

[Diana Picus via Phabricator via cfe-commits]

rovka added inline comments.



Comment at: flang/include/flang/Frontend/FrontendOptions.h:37
 
+  /// Emit a .llvm file
+  EmitLLVM,

Shouldn't this be .ll?



Comment at: flang/lib/Frontend/FrontendActions.cpp:421
+  // Set-up the MLIR pass manager
+  fir::setTargetTriple(*mlirModule_, "native");
+  auto  = ci.invocation().semanticsContext().defaultKinds();

Nit: Should we assert that mlirModule exists?
Also, why doesn't it already have a triple and a kind mapping?



Comment at: flang/lib/Frontend/FrontendActions.cpp:464
+  if (ci.IsOutputStreamNull()) {
+// Lower from the LLVM dialect to LLVM IR
+os = ci.CreateDefaultOutputFile(

This looks like an obsolete comment.



Comment at: flang/test/Driver/emit-llvm.f90:1
+! The the `-emit-llvm` option
+




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119012

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

[PATCH] D78477: [profile] Don't crash when forking in several threads

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

Ping!
https://bugs.llvm.org/show_bug.cgi?id=46092


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78477



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

[PATCH] D78477: [profile] Don't crash when forking in several threads

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

Hi! I think this commit is causing some instability on the 10.0.1 branch on 
32-bit arm: https://bugs.llvm.org/show_bug.cgi?id=46092 
Can you have a look?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78477



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

[PATCH] D53137: Scalable vector core instruction support + size queries

[Diana Picus via Phabricator via cfe-commits]

rovka accepted this revision.
rovka marked 2 inline comments as done.
rovka added a comment.
This revision is now accepted and ready to land.

This looks good to me, maybe wait a while to see if anyone else has any further 
comments.




Comment at: llvm/include/llvm/IR/DataLayout.h:454
+auto BaseSize = getTypeSizeInBits(Ty);
+return { (BaseSize.getKnownMinSize() + 7) / 8, BaseSize.isScalable() };
   }

huntergr wrote:
> rovka wrote:
> > We already overload operator /, why not overload + as well so we don't have 
> > to change the body of this method?
> Scaling a size with * or / has a clear meaning to me, since it's independent 
> of vscale; getting a vector that's half the size or four times larger just 
> works.
> 
> Using + (or -) on the other hand doesn't seem to be as clear; I wasn't sure 
> if a standalone int should be automatically treated as being the same as the 
> TypeSize, or always considered Fixed. If we try for the former I can imagine 
> quite a few bugs arising.
> 
> I could add a roundBitsToNearestByteSize method to move the arithmetic 
> elsewhere if that would be acceptable?
You're right, + on TypeSizes would be confusing. This looks ok as-is then, no 
need to fiddle with it more.



Comment at: llvm/include/llvm/IR/DataLayout.h:656
+ 
getTypeSizeInBits(VTy->getElementType()).getKnownMinSize();
+return ScalableSize(MinBits, EltCnt.Scalable);
   }

huntergr wrote:
> rovka wrote:
> > Maybe just return VTy->getElementCount() * 
> > getTypeSizeInBits(VTy->getElementType()).getFixedSize().
> There's no support for generating a TypeSize from an ElementCount in that 
> way; is that an interface you feel is useful?
> 
> (I'll certainly change the `getKnownMinSize` to `getFixedSize` though, since 
> we're just referring to a scalar)
Actually, no, now that I think about it a bit more it might be clearer to spell 
it out this way.



Comment at: llvm/include/llvm/Support/TypeSize.h:122
+  // Use in places where a scalable size doesn't make sense (e.g. non-vector
+  // types, or vectors in backends which don't support scalable vectors)
+  uint64_t getFixedSize() const {

Microscopic nit: punctuation.



Comment at: llvm/include/llvm/Support/TypeSize.h:143
+  // NOTE: This interface is obsolete and will be removed in a future version
+  // of LLVM in favour of calling getFixedSize() directly
+  operator uint64_t() const {

Ditto.



Comment at: llvm/include/llvm/Support/TypeSize.h:148
+
+  // Additional convenience operators needed to avoid ambiguous parses
+  // TODO: Make uint64_t the default operator?

Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D53137



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

[PATCH] D53137: Scalable vector core instruction support + size queries

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

> I suspect 'ScalableSize' is the wrong term now; 'TypeSize' may be better. 
> Thoughts?

I agree, TypeSize sounds better. Maybe we can replace the public constructor 
with 2 static methods, TypeSize::Fixed(Size) and TypeSize::Scalable(Size), so 
we don't always have to spell out /* Scalable =*/.




Comment at: llvm/include/llvm/IR/DataLayout.h:454
+auto BaseSize = getTypeSizeInBits(Ty);
+return { (BaseSize.getKnownMinSize() + 7) / 8, BaseSize.isScalable() };
   }

We already overload operator /, why not overload + as well so we don't have to 
change the body of this method?



Comment at: llvm/include/llvm/IR/DataLayout.h:487
+auto BaseSize = getTypeStoreSize(Ty);
+uint64_t MinAlignedSize = alignTo(BaseSize.getKnownMinSize(),
+  getABITypeAlignment(Ty));

Can we add a version of alignTo that works with ScalableSize instead?



Comment at: llvm/include/llvm/IR/DataLayout.h:656
+ 
getTypeSizeInBits(VTy->getElementType()).getKnownMinSize();
+return ScalableSize(MinBits, EltCnt.Scalable);
   }

Maybe just return VTy->getElementCount() * 
getTypeSizeInBits(VTy->getElementType()).getFixedSize().


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D53137



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

[PATCH] D67549: [IntrinsicEmitter] Add overloaded types for SVE intrinsics (Subdivide2 & Subdivide4)

[Diana Picus via Phabricator via cfe-commits]

rovka added inline comments.



Comment at: include/llvm/IR/DerivedTypes.h:515
+  else
+VTy = VectorType::getTruncatedElementVectorType(VTy);
+}

Why not move this logic to getTruncatedElementVectorType and drop 
getNarrowerFpElementVectorType altogether? 



Comment at: include/llvm/IR/Intrinsics.h:130
+ Kind == PtrToElt || Kind == VecElementArgument ||
+ Kind == Subdivide2Argument || Kind == Subdivide4Argument);
   return Argument_Info >> 3;

Not directly related to your change: Is there a reason why VecOfAnyPtrsToElt 
isn't in this assert? If not, maybe this is a good time to add a 
First/LastArgumentKind and just check that Kind is in the range.



Comment at: lib/IR/Function.cpp:989
+Type *Ty = Tys[D.getArgumentNumber()];
+if (VectorType *VTy = dyn_cast(Ty))
+  return VectorType::getSubdividedVectorType(VTy, 1);

Maybe replace this with an assert and drop the unreachable.



Comment at: lib/IR/Function.cpp:997
+  return VectorType::getSubdividedVectorType(VTy, 2);
+llvm_unreachable("unhandled");
+  }

Ditto,



Comment at: lib/IR/Function.cpp:1312
+case IITDescriptor::Subdivide4Argument: {
+  // This may only be used when referring to a previous vector argument.
+  if (D.getArgumentNumber() >= ArgTys.size())

Can you share this code? Either a small helper or a fallthrough with a repeat 
check on D.Kind to get the number of subdivisions would work.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67549



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

[PATCH] D66524: [SVE][Inline-Asm] Add constraints for SVE predicate registers

[Diana Picus via Phabricator via cfe-commits]

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

I think all the outstanding comments have been addressed. LGTM.


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

https://reviews.llvm.org/D66524



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

[PATCH] D66524: [SVE][Inline-Asm] Add constraints for SVE predicate registers

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

Just some drive-by suggestions :)




Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:5747
+
+PredicateConstraint isPredicateConstraint(StringRef Constraint) {
+  PredicateConstraint P = PredicateConstraint::Invalid;

Nit: I think get- or parsePredicateConstraint reads better, since this doesn't 
return a simple yes/no answer.



Comment at: test/CodeGen/AArch64/aarch64-sve-asm.ll:50
+; CHECK: [[ARG3:%[0-9]+]]:ppr = COPY $p0
+; CHECK: [[ARG4:%[0-9]+]]:ppr_3b = COPY [[ARG3]]
+define  @test_svfadd_f16( %Pg,  %Zn,  %Zm) {

Nit: I would be a bit pedantic here and also check that they are used for the 
inline asm.


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

https://reviews.llvm.org/D66524



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

[PATCH] D64739: [SVE][Inline-Asm] Add support to specify SVE registers in the clobber list

[Diana Picus via Phabricator via cfe-commits]

rovka added inline comments.



Comment at: clang/test/CodeGen/aarch64-sve-inline-asm.c:1
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -emit-llvm -target-feature 
+sve -o - %s | FileCheck %s
+

sdesmalen wrote:
> rovka wrote:
> > Can you also add a test without +sve, to make sure we get a diagnostic?
> Without the `-emit-llvm` part this test invokes (and tests the diagnostic of) 
> the compiler. I don't think this is what we want. At the same time, this code 
> should probably still continue match the z and p registers even if the target 
> feature is not given, and thus leave it to LLVM to determine whether the use 
> of these registers makes sense or not. So removing `-target-feature +sve` 
> from the RUN line should be sufficient here. @rovka do you agree?
Good point, we probably don't want this to be an integration test of the whole 
compiler. Removing the target feature altogether sounds good to me.


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

https://reviews.llvm.org/D64739



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

[PATCH] D62960: Add SVE opaque built-in types

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

FWIW, I think the tests look great. Would be nice if someone more experienced 
with clang could also have a look though.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62960



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

[PATCH] D64739: [SVE][Inline-Asm] Add support to clang for SVE inline assembly

[Diana Picus via Phabricator via cfe-commits]

rovka added inline comments.



Comment at: clang/lib/Basic/Targets/AArch64.cpp:307
 
-// 32-bit floating point regsisters
+// 32-bit floating point registers
 "s0", "s1", "s2", "s3", "s4", "s5", "s6", "s7", "s8", "s9", "s10", "s11",

You should commit the typo fixes separately.



Comment at: clang/test/CodeGen/aarch64-sve-inline-asm.c:1
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -emit-llvm -target-feature 
+sve -o - %s | FileCheck %s
+

Can you also add a test without +sve, to make sure we get a diagnostic?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64739



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

[PATCH] D62960: SVE opaque type for C intrinsics demo

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

This looks much better, thanks! Shouldn't there be more tests, e.g. for 
mangling and maybe the ASTImporter?


Repository:
  rC Clang

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

https://reviews.llvm.org/D62960



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

[PATCH] D62960: SVE opaque type for C intrinsics demo

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

Just a few nits/suggestions.




Comment at: include/clang/Basic/AArch64SVEACLETypes.def:10
+//
+//  This file defines the database about various builtin singleton types.
+//

You can be more specific :)



Comment at: include/clang/Basic/AArch64SVEACLETypes.def:29
+#define SVE_VECTOR_TYPE(Name, Id, SingletonId, ElKind, ElBits, IsSigned, IsFP)\
+  BUILTIN_TYPE(Name, Id, SingletonId)
+#endif

Maybe use SVE_TYPE instead of BUILTIN_TYPE, to avoid any confusion?  You can't 
re-use a defition of BUILTIN_TYPE between this header and BuiltinTypes.def 
anyway, since they both undefine it at the end.



Comment at: include/clang/Serialization/ASTBitCodes.h:1021
 #include "clang/Basic/OpenCLExtensionTypes.def"
+// SVE Types
+#define SVE_VECTOR_TYPE(Name, Id, SingletonId, ElKind, ElBits, IsSigned, IsFP)\

super nit: /// \brief SVE types with auto numeration



Comment at: lib/AST/ASTContext.cpp:6645
+  case BuiltinType::Id:
+#include "clang/Basic/AArch64SVEACLETypes.def"
 #define BUILTIN_TYPE(KIND, ID)

Here and in most other places: no need for 2 defines, you can just #define 
BUILTIN_TYPE (or if you choose to rename it to SVE_TYPE) since it's the same 
code for both vectors and predicates.



Comment at: lib/AST/ItaniumMangle.cpp:2680
+break;
+#include "clang/Basic/AArch64SVEACLETypes.def"
   }

erik.pilkington wrote:
> rsandifo-arm wrote:
> > erik.pilkington wrote:
> > > jfb wrote:
> > > > @rjmccall you probably should review this part.
> > > Sorry for the drive by comment, but: All of these mangling should really 
> > > be using the "vendor extension" production IMO:
> > > 
> > > ` ::= u `
> > > 
> > > As is, these manglings intrude on the users's namespace, (i.e. if they 
> > > had a type named `objc_selector` or something), and confuse demanglers 
> > > which incorrectly assume these are substitutable (vendor extension 
> > > builtin types are substitutable too though, but that should be handled 
> > > here).
> > It isn't obvious from the patch, but the SVE names that we're mangling are 
> > predefined names like __SVInt8_t. rather than user-facing names like 
> > svint8_t  The predefined names and their mangling are defined by the 
> > platform ABI (https://developer.arm.com/docs/100986/), so it wouldn't 
> > be valid for another part of the implementation to use those names for 
> > something else.
> > 
> > I realise you were making a general point here though, sorry.
> > 
> The mangling in the document you linked does use the vendor extension 
> production here though, i.e. the example is `void f(int8x8_t)`, which mangles 
> to _Z1f**u10__Int8x8_t**. It is true that this shouldn't ever collide with 
> another mangling in practice, but my point is there isn't any need to smuggle 
> it into the mangling by pretending it's a user defined type, when the itanium 
> grammar and related tools have a special way for vendors to add builtin types.
I agree with Erik here, the example in the PCS document seems to suggest using 
u. I think either the patch needs to be updated or the document needs to be 
more clear about what the mangling is supposed to look like.



Comment at: lib/AST/MicrosoftMangle.cpp:2110
+llvm_unreachable("mangling an sve type not yet supported");
+#include "clang/Basic/AArch64SVEACLETypes.def"
   }

Should we have llvm_unreachable or report a proper error? I like the 
unreachable if we're checking elsewhere that SVE isn't supported on Windows, 
but I notice we report errors for some of the other types.



Comment at: lib/CodeGen/CGDebugInfo.cpp:709
+  case BuiltinType::Id: \
+return nullptr;
+#include "clang/Basic/AArch64SVEACLETypes.def"

I don't really know this code, but I can't help but notice that nullptr is only 
ever used for the void type. Is it safe to also use it for the SVE types, or 
can we do something else instead?


Repository:
  rC Clang

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

https://reviews.llvm.org/D62960



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

[PATCH] D35884: Update to use enum classes for various ARM *Kind enums

[Diana Picus via Phabricator via cfe-commits]

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

Looks entirely mechanical, I don't see any problem with it (just a couple of 
nits).




Comment at: lib/Basic/Targets/AArch64.cpp:94
  llvm::AArch64::parseCPUArch(Name) !=
- static_cast(llvm::AArch64::ArchKind::AK_INVALID);
+ llvm::AArch64::ArchKind::INVALID;
 }

My eyes might be deceiving me, but did you run clang-format?



Comment at: lib/Driver/ToolChains/Darwin.cpp:71
   const llvm::Triple::ArchType Arch = getArchTypeForMachOArchName(Str);
-  unsigned ArchKind = llvm::ARM::parseArch(Str);
+  llvm::ARM::ArchKind AK = llvm::ARM::parseArch(Str);
   T.setArch(Arch);

Nitpick: The rename seems unnecessary (and anyway the file is inconsistent 
between Arch, ArchKind and AK).


https://reviews.llvm.org/D35884



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

[PATCH] D31709: [NFC] Refactor DiagnosticRenderer to use FullSourceLoc

[Diana Picus via Phabricator via cfe-commits]

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

I don't see anything wrong with this, I think you can commit it in a couple of 
days if nobody comes up with a reason why the DiagnosticRenderer shouldn't use 
FullSourceLoc.


https://reviews.llvm.org/D31709



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