[PATCH] D99353: [driver] Make `clang` warn rather then error on `flang` options

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski abandoned this revision.
awarzynski added a comment.
Herald added a subscriber: MaskRay.
Herald added a project: All.

With the imminent switch to GitHub and no new comments in over 2 years, I feel 
that it's time to abandon this change. Feel free to re-use this in the future :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99353

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

[PATCH] D158763: [flang][driver] Mark -L as visible in Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

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

LGTM, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158763

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

[PATCH] D158612: [flang][driver] Ensure negative flags have the same visibility as positive

[Andrzej Warzynski via Phabricator via cfe-commits]

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

LGTM, ta!




Comment at: clang/include/clang/Driver/Options.td:5314
   NegFlag,
-  PosFlag,
+  PosFlag,
   BothFlags<[], [ClangOption], " the integrated assembler">>;

Could you add a test? 
https://github.com/llvm/llvm-project/blob/main/flang/test/Driver/fno-integrated-as.f90

I would probably rename that file as `integrated-as.f90` and test 3 variants:
```
! RUN: 
! RUN: -fintegrated-as
! RUN: -fno-integrated-as
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158612

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

[PATCH] D158507: [Flang][Driver] Add support for fomit-frame-pointer

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

> Temporary fix for unknown argument error '-fomit-frame-pointer' when running 
> flang tests

I don't follow - there's quite a lot going on here. More than the summary 
suggests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158507

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

[PATCH] D158307: [flang][driver] Disable Clang options in Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb16a7581ff8b: [flang][driver] Disable Clang options in Flang 
(authored by awarzynski).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158307

Files:
  clang/lib/Driver/Driver.cpp


Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -6495,7 +6495,7 @@
   if (IsDXCMode())
 return llvm::opt::Visibility(options::DXCOption);
   if (IsFlangMode())  {
-return llvm::opt::Visibility(options::ClangOption | options::FlangOption);
+return llvm::opt::Visibility(options::FlangOption);
   }
   return llvm::opt::Visibility(options::ClangOption);
 }


Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -6495,7 +6495,7 @@
   if (IsDXCMode())
 return llvm::opt::Visibility(options::DXCOption);
   if (IsFlangMode())  {
-return llvm::opt::Visibility(options::ClangOption | options::FlangOption);
+return llvm::opt::Visibility(options::FlangOption);
   }
   return llvm::opt::Visibility(options::ClangOption);
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158309: [flang][driver] Mark -Wl as visible in Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3758aed31a51: [flang][driver] Mark -Wl as visible in Flang 
(authored by awarzynski).

Changed prior to commit:
  https://reviews.llvm.org/D158309?vs=551620=551912#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158309

Files:
  clang/include/clang/Driver/Options.td
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-help.f90
  flang/test/Driver/misc-flags.f90


Index: flang/test/Driver/misc-flags.f90
===
--- flang/test/Driver/misc-flags.f90
+++ flang/test/Driver/misc-flags.f90
@@ -1,6 +1,9 @@
 ! Make sure that `-l` is "visible" to Flang's driver
 ! RUN: %flang -lpgmath -### %s
 
+! Make sure that `-Wl` is "visible" to Flang's driver
+! RUN: %flang -Wl,abs -### %s
+
 program hello
   write(*,*), "Hello world!"
 end program hello
Index: flang/test/Driver/driver-help.f90
===
--- flang/test/Driver/driver-help.f90
+++ flang/test/Driver/driver-help.f90
@@ -120,6 +120,7 @@
 ! HELP-NEXT: -U   Undefine macro 
 ! HELP-NEXT: --version   Print version information
 ! HELP-NEXT: -v  Show commands to run and use verbose 
output
+! HELP-NEXT: -Wl,   Pass the comma separated arguments in 
 to the linker
 ! HELP-NEXT: -W Enable the specified warning
 ! HELP-NEXT: -XflangPass  to the flang compiler
 ! HELP-NEXT: -xTreat subsequent input files as having 
type 
Index: flang/test/Driver/driver-help-hidden.f90
===
--- flang/test/Driver/driver-help-hidden.f90
+++ flang/test/Driver/driver-help-hidden.f90
@@ -132,6 +132,7 @@
 ! CHECK-NEXT: -U   Undefine macro 
 ! CHECK-NEXT: --version   Print version information
 ! CHECK-NEXT: -v  Show commands to run and use verbose 
output
+! CHECK-NEXT: -Wl,   Pass the comma separated arguments in 
 to the linker
 ! CHECK-NEXT: -W Enable the specified warning
 ! CHECK-NEXT: -XflangPass  to the flang compiler
 ! CHECK-NEXT: -xTreat subsequent input files as having 
type 
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -912,7 +912,8 @@
   HelpText<"Enable warnings for deprecated constructs and define 
__DEPRECATED">;
 def Wno_deprecated : Flag<["-"], "Wno-deprecated">, Group,
   Visibility<[ClangOption, CC1Option]>;
-def Wl_COMMA : CommaJoined<["-"], "Wl,">, Flags<[LinkerInput, RenderAsInput]>,
+def Wl_COMMA : CommaJoined<["-"], "Wl,">, Visibility<[ClangOption, 
FlangOption]>,
+  Flags<[LinkerInput, RenderAsInput]>,
   HelpText<"Pass the comma separated arguments in  to the linker">,
   MetaVarName<"">, Group;
 // FIXME: This is broken; these should not be Joined arguments.


Index: flang/test/Driver/misc-flags.f90
===
--- flang/test/Driver/misc-flags.f90
+++ flang/test/Driver/misc-flags.f90
@@ -1,6 +1,9 @@
 ! Make sure that `-l` is "visible" to Flang's driver
 ! RUN: %flang -lpgmath -### %s
 
+! Make sure that `-Wl` is "visible" to Flang's driver
+! RUN: %flang -Wl,abs -### %s
+
 program hello
   write(*,*), "Hello world!"
 end program hello
Index: flang/test/Driver/driver-help.f90
===
--- flang/test/Driver/driver-help.f90
+++ flang/test/Driver/driver-help.f90
@@ -120,6 +120,7 @@
 ! HELP-NEXT: -U   Undefine macro 
 ! HELP-NEXT: --version   Print version information
 ! HELP-NEXT: -v  Show commands to run and use verbose output
+! HELP-NEXT: -Wl,   Pass the comma separated arguments in  to the linker
 ! HELP-NEXT: -W Enable the specified warning
 ! HELP-NEXT: -XflangPass  to the flang compiler
 ! HELP-NEXT: -xTreat subsequent input files as having type 
Index: flang/test/Driver/driver-help-hidden.f90
===
--- flang/test/Driver/driver-help-hidden.f90
+++ flang/test/Driver/driver-help-hidden.f90
@@ -132,6 +132,7 @@
 ! CHECK-NEXT: -U   Undefine macro 
 ! CHECK-NEXT: --version   Print version information
 ! CHECK-NEXT: -v  Show commands to run and use verbose output
+! CHECK-NEXT: -Wl,   Pass the comma separated arguments in  to the linker
 ! CHECK-NEXT: -W Enable the specified warning
 ! CHECK-NEXT: -XflangPass  to the flang compiler
 ! CHECK-NEXT: -xTreat 

[PATCH] D158309: [flang][driver] Mark -Wl as visible in Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

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

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158309

Files:
  clang/include/clang/Driver/Options.td
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-help.f90
  flang/test/Driver/misc-flags.f90


Index: flang/test/Driver/misc-flags.f90
===
--- flang/test/Driver/misc-flags.f90
+++ flang/test/Driver/misc-flags.f90
@@ -1,6 +1,9 @@
 ! Make sure that `-l` is "visible" to Flang's driver
 ! RUN: %flang -lpgmath -### %s
 
+! Make sure that `-Wl` is "visible" to Flang's driver
+! RUN: %flang -Wl,abs -### %s
+
 program hello
   write(*,*), "Hello world!"
 end program hello
Index: flang/test/Driver/driver-help.f90
===
--- flang/test/Driver/driver-help.f90
+++ flang/test/Driver/driver-help.f90
@@ -120,6 +120,7 @@
 ! HELP-NEXT: -U   Undefine macro 
 ! HELP-NEXT: --version   Print version information
 ! HELP-NEXT: -v  Show commands to run and use verbose 
output
+! HELP-NEXT: -Wl, Pass the comma separated arguments in  to the 
linker
 ! HELP-NEXT: -W Enable the specified warning
 ! HELP-NEXT: -XflangPass  to the flang compiler
 ! HELP-NEXT: -xTreat subsequent input files as having 
type 
Index: flang/test/Driver/driver-help-hidden.f90
===
--- flang/test/Driver/driver-help-hidden.f90
+++ flang/test/Driver/driver-help-hidden.f90
@@ -132,6 +132,7 @@
 ! CHECK-NEXT: -U   Undefine macro 
 ! CHECK-NEXT: --version   Print version information
 ! CHECK-NEXT: -v  Show commands to run and use verbose 
output
+! CHECK-NEXT: -Wl, Pass the comma separated arguments in  to the 
linker
 ! CHECK-NEXT: -W Enable the specified warning
 ! CHECK-NEXT: -XflangPass  to the flang compiler
 ! CHECK-NEXT: -xTreat subsequent input files as having 
type 
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -912,7 +912,8 @@
   HelpText<"Enable warnings for deprecated constructs and define 
__DEPRECATED">;
 def Wno_deprecated : Flag<["-"], "Wno-deprecated">, Group,
   Visibility<[ClangOption, CC1Option]>;
-def Wl_COMMA : CommaJoined<["-"], "Wl,">, Flags<[LinkerInput, RenderAsInput]>,
+def Wl_COMMA : CommaJoined<["-"], "Wl,">, Visibility<[ClangOption, 
FlangOption]>,
+  Flags<[LinkerInput, RenderAsInput]>,
   HelpText<"Pass the comma separated arguments in  to the linker">,
   MetaVarName<"">, Group;
 // FIXME: This is broken; these should not be Joined arguments.


Index: flang/test/Driver/misc-flags.f90
===
--- flang/test/Driver/misc-flags.f90
+++ flang/test/Driver/misc-flags.f90
@@ -1,6 +1,9 @@
 ! Make sure that `-l` is "visible" to Flang's driver
 ! RUN: %flang -lpgmath -### %s
 
+! Make sure that `-Wl` is "visible" to Flang's driver
+! RUN: %flang -Wl,abs -### %s
+
 program hello
   write(*,*), "Hello world!"
 end program hello
Index: flang/test/Driver/driver-help.f90
===
--- flang/test/Driver/driver-help.f90
+++ flang/test/Driver/driver-help.f90
@@ -120,6 +120,7 @@
 ! HELP-NEXT: -U   Undefine macro 
 ! HELP-NEXT: --version   Print version information
 ! HELP-NEXT: -v  Show commands to run and use verbose output
+! HELP-NEXT: -Wl, Pass the comma separated arguments in  to the linker
 ! HELP-NEXT: -W Enable the specified warning
 ! HELP-NEXT: -XflangPass  to the flang compiler
 ! HELP-NEXT: -xTreat subsequent input files as having type 
Index: flang/test/Driver/driver-help-hidden.f90
===
--- flang/test/Driver/driver-help-hidden.f90
+++ flang/test/Driver/driver-help-hidden.f90
@@ -132,6 +132,7 @@
 ! CHECK-NEXT: -U   Undefine macro 
 ! CHECK-NEXT: --version   Print version information
 ! CHECK-NEXT: -v  Show commands to run and use verbose output
+! CHECK-NEXT: -Wl, Pass the comma separated arguments in  to the linker
 ! CHECK-NEXT: -W Enable the specified warning
 ! CHECK-NEXT: -XflangPass  to the flang compiler
 ! CHECK-NEXT: -xTreat subsequent input files as having type 
Index: clang/include/clang/Driver/Options.td
===
--- 

[PATCH] D158307: [flang][driver] Disable Clang options in Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

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

Restore the desired setting that was reverted in

  https://reviews.llvm.org/D158289.

This is to be merged once Fortran tests in llvm-test-suite are updated
accordingly.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158307

Files:
  clang/lib/Driver/Driver.cpp


Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -6495,7 +6495,7 @@
   if (IsDXCMode())
 return llvm::opt::Visibility(options::DXCOption);
   if (IsFlangMode())  {
-return llvm::opt::Visibility(options::ClangOption | options::FlangOption);
+return llvm::opt::Visibility(options::FlangOption);
   }
   return llvm::opt::Visibility(options::ClangOption);
 }


Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -6495,7 +6495,7 @@
   if (IsDXCMode())
 return llvm::opt::Visibility(options::DXCOption);
   if (IsFlangMode())  {
-return llvm::opt::Visibility(options::ClangOption | options::FlangOption);
+return llvm::opt::Visibility(options::FlangOption);
   }
   return llvm::opt::Visibility(options::ClangOption);
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158289: [flang][driver] Partial revert of https://reviews.llvm.org/D157837

[Andrzej Warzynski via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG89053e495903: [flang][driver] Partial revert of D157837 
(authored by awarzynski).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158289

Files:
  clang/lib/Driver/Driver.cpp


Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -6494,8 +6494,8 @@
 return llvm::opt::Visibility(options::CLOption);
   if (IsDXCMode())
 return llvm::opt::Visibility(options::DXCOption);
-  if (IsFlangMode()) {
-return llvm::opt::Visibility(options::FlangOption);
+  if (IsFlangMode())  {
+return llvm::opt::Visibility(options::ClangOption | options::FlangOption);
   }
   return llvm::opt::Visibility(options::ClangOption);
 }


Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -6494,8 +6494,8 @@
 return llvm::opt::Visibility(options::CLOption);
   if (IsDXCMode())
 return llvm::opt::Visibility(options::DXCOption);
-  if (IsFlangMode()) {
-return llvm::opt::Visibility(options::FlangOption);
+  if (IsFlangMode())  {
+return llvm::opt::Visibility(options::ClangOption | options::FlangOption);
   }
   return llvm::opt::Visibility(options::ClangOption);
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158289: [flang][driver] Partial revert of https://reviews.llvm.org/D157837

[Andrzej Warzynski via Phabricator via cfe-commits]

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

This is a partial revert of https://reviews.llvm.org/D157837 - it turns
out that the LLVM test suite needs to be updated first not to use any of
the unsupported Flang options:

- https://github.com/llvm/llvm-test-suite

Sample errors:

  flang-new: error: unknown argument: '-fbounds-check'
  flang-new: error: unknown argument: '-fcheck=all'
  flang-new: error: unknown argument: '-fcheck-array-temporaries'

Once the test suite is updated, we can restore the reverted setting.

Broken bot:

- https://lab.llvm.org/buildbot/#/builders/197/builds/9001


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158289

Files:
  clang/lib/Driver/Driver.cpp


Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -6494,8 +6494,8 @@
 return llvm::opt::Visibility(options::CLOption);
   if (IsDXCMode())
 return llvm::opt::Visibility(options::DXCOption);
-  if (IsFlangMode()) {
-return llvm::opt::Visibility(options::FlangOption);
+  if (IsFlangMode())  {
+return llvm::opt::Visibility(options::ClangOption | options::FlangOption);
   }
   return llvm::opt::Visibility(options::ClangOption);
 }


Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -6494,8 +6494,8 @@
 return llvm::opt::Visibility(options::CLOption);
   if (IsDXCMode())
 return llvm::opt::Visibility(options::DXCOption);
-  if (IsFlangMode()) {
-return llvm::opt::Visibility(options::FlangOption);
+  if (IsFlangMode())  {
+return llvm::opt::Visibility(options::ClangOption | options::FlangOption);
   }
   return llvm::opt::Visibility(options::ClangOption);
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157837: [flang][driver] Update the visibility of Clang options in Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG83a06997c69a: [flang][driver] Update the visibility of Clang 
options in Flang (authored by awarzynski).

Changed prior to commit:
  https://reviews.llvm.org/D157837?vs=550668=551220#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157837

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/flang/flang.f90
  clang/test/Driver/flang/flang_ucase.F90
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-help.f90
  flang/test/Driver/fast_math.f90

Index: flang/test/Driver/fast_math.f90
===
--- flang/test/Driver/fast_math.f90
+++ flang/test/Driver/fast_math.f90
@@ -70,7 +70,7 @@
 ! UNSUPPORTED: system-windows
 ! UNSUPPORTED: target=powerpc{{.*}}
 ! RUN: %flang -ffast-math -### %s -o %t 2>&1 \
-! RUN:   --target=x86_64-unknown-linux -no-pie --gcc-toolchain="" \
+! RUN:   --target=x86_64-unknown-linux -no-pie \
 ! RUN:   --sysroot=%S/../../../clang/test/Driver/Inputs/basic_linux_tree \
 ! RUN: | FileCheck --check-prefix=CHECK-CRT %s
 ! CHECK-CRT: {{crtbegin.?\.o}}
Index: flang/test/Driver/driver-help.f90
===
--- flang/test/Driver/driver-help.f90
+++ flang/test/Driver/driver-help.f90
@@ -14,224 +14,242 @@
 ! HELP:USAGE: flang
 ! HELP-EMPTY:
 ! HELP-NEXT:OPTIONS:
-! HELP-NEXT: -###   Print (but do not run) the commands to run for this compilation
-! HELP-NEXT: -cpp   Enable predefined and command line preprocessor macros
-! HELP-NEXT: -c Only run preprocess, compile, and assemble steps
-! HELP-NEXT: -D = Define  to  (or 1 if  omitted)
-! HELP-NEXT: -emit-llvm Use the LLVM representation for assembler and object files
-! HELP-NEXT: -E Only run the preprocessor
+! HELP-NEXT: -###Print (but do not run) the commands to run for this compilation
+! HELP-NEXT: -cppEnable predefined and command line preprocessor macros
+! HELP-NEXT: -c  Only run preprocess, compile, and assemble steps
+! HELP-NEXT: -D =  Define  to  (or 1 if  omitted)
+! HELP-NEXT: -emit-llvm  Use the LLVM representation for assembler and object files
+! HELP-NEXT: -E  Only run the preprocessor
 ! HELP-NEXT: -falternative-parameter-statement
-! HELP-NEXT: Enable the old style PARAMETER statement
-! HELP-NEXT: -fapprox-func  Allow certain math function calls to be replaced with an approximately equivalent calculation
-! HELP-NEXT: -fbackslashSpecify that backslash in string introduces an escape character
-! 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
-! HELP-NEXT: -fdefault-integer-8Set the default integer and logical kind to an 8 byte wide type
-! HELP-NEXT: -fdefault-real-8   Set the default real kind to an 8 byte wide type
-! HELP-NEXT: -ffast-mathAllow aggressive, lossy floating-point optimizations
-! HELP-NEXT: -ffixed-form   Process source files in fixed form
+! HELP-NEXT: Enable the old style PARAMETER statement
+! HELP-NEXT: -fapprox-func   Allow certain math function calls to be replaced with an approximately equivalent calculation
+! HELP-NEXT: -fbackslash Specify that backslash in string introduces an escape character
+! HELP-NEXT: -fcolor-diagnostics Enable 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
+! HELP-NEXT: -fdefault-integer-8 Set the default integer and logical kind to an 8 byte wide type
+! HELP-NEXT: -fdefault-real-8Set the default real kind to an 8 byte wide type
+! HELP-NEXT: -ffast-math Allow aggressive, lossy floating-point optimizations
+! HELP-NEXT: -ffixed-formProcess source files in fixed form
 ! HELP-NEXT: -ffixed-line-length=
-! HELP-NEXT: Use  as character line width in fixed mode
-! HELP-NEXT: -ffp-contract= Form fused FP ops (e.g. FMAs)
-! HELP-NEXT: -ffree-formProcess source files in free form
-! HELP-NEXT: -fimplicit-noneNo implicit typing allowed unless overridden by IMPLICIT statements
+! HELP-NEXT: Use  as character line width in fixed mode
+! HELP-NEXT: -ffp-contract=   Form fused FP ops (e.g. FMAs)
+! HELP-NEXT: -ffree-form Process source files in free 

[PATCH] D156320: [Flang][Driver] Add support for Rpass and related options

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Victor, this is proving quite tricky to review. There's already been a lot of 
updates and many of them are summarized as either "code refactor" or 
"clean-up". Please reduce traffic/noise and use more descriptive summaries.

Also, rather than adding new features in this already large change (I am 
referring to e.g. `DK_MachineOptimizationRemarkMissed`), please try to identify 
ways to split this patch further. Here are some suggestions (I've also made 
comments inline):

1. In the first iteration (like you effectively do now), focus on  OPT_R_Joined 

 options (e.g. `-Rpass`, `Rpass-analysis`, `-Rpass-missed`). Focus on basic 
functionality that demonstrates that correct information is returned from the 
backend. No need to fine tune the remarks with e.g. full file path or relevant 
remark option.
2. Next, add support for -Rpass=/-Rpass-missed=/-Rpass-analysis= 
.
 That's when e.g. `llvm::opt::OptSpecifier optEq` in `parseOptimizationRemark` 
would be needed (but not in Step 1).
3. Fine tune how the report is printed (e.g. improve file name by adding full 
path, add valid remark option at the end etc).
4. Add support for machine optimisations, e.g. 
`DK_MachineOptimizationRemarkMissed`.

This is easily 4 patches ;-)




Comment at: flang/lib/Frontend/CompilerInvocation.cpp:158
+parseOptimizationRemark(clang::DiagnosticsEngine ,
+llvm::opt::ArgList , llvm::opt::OptSpecifier 
optEq,
+llvm::StringRef name) {

`optEq` is not used, but it should be.



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:244-246
+  // Get -Rpass option regex. If empty, "".*"" is used. From all successful
+  // optimization passes applied, the regex will return only pass names that
+  // match it.

This is for `-Rpass=`, which is not tested. And the comment is inaccurate.



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:250-252
+  // Get -Rpass-missed option regex. If empty, "".*"" is used. From all
+  // optimization passes that failed to be applied, the regex will return only
+  // pass names that match it.

This is for `-Rpass-missed=`, which is not tested. And the comment is 
inaccurate.



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:256-257
+
+  // Specify which passes, with additional information,
+  // should be reported using a regex.
+  opts.OptimizationRemarkAnalysis = parseOptimizationRemark(

This is for `-Rpass-analysis=`, which is not tested. And the comment is 
inaccurate.



Comment at: flang/lib/Frontend/FrontendActions.cpp:1032-1043
+case llvm::DK_MachineOptimizationRemark:
+  optimizationRemarkHandler(
+  llvm::cast(di));
+  break;
+case llvm::DK_MachineOptimizationRemarkMissed:
+  optimizationRemarkHandler(
+  llvm::cast(di));

victorkingi wrote:
> This cases should handle back-end passes as the previous cases only handled 
> middle-end
Please move this to a dedicated patch with a test for each of these cases.



Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:19
 #include "flang/Frontend/TextDiagnostic.h"
+#include "string"
 #include "clang/Basic/DiagnosticOptions.h"

https://llvm.org/docs/CodingStandards.html#include-style
 
>   1.  Main Module Header
>   2.  Local/Private Headers
>   3.  LLVM project/subproject headers (clang/..., lldb/..., llvm/..., etc)
>   4.  System #includes




Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:39-54
+static void printRemarkOption(llvm::raw_ostream ,
+  clang::DiagnosticsEngine::Level level,
+  const clang::Diagnostic ) {
+  llvm::StringRef opt =
+  clang::DiagnosticIDs::getWarningOptionForDiag(info.getID());
+  if (!opt.empty()) {
+// We still need to check if the level is a Remark since, an unknown option

Move to a dedicated patch.



Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:76-105
+  // split incoming string to get the absolute path and filename in the
+  // case we are receiving optimization remarks from BackendRemarkConsumer
+  std::string diagMsg = std::string(diagMessageStream.str());
+  std::string delimiter = ";;";
+
+  size_t pos = 0;
+  llvm::SmallVector tokens;

Move to a dedicated patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156320

___
cfe-commits mailing list

[PATCH] D157837: [flang][driver] Update the visibility of Clang options in Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

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

Rebase on top of main


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157837

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-help.f90
  flang/test/Driver/fast_math.f90

Index: flang/test/Driver/fast_math.f90
===
--- flang/test/Driver/fast_math.f90
+++ flang/test/Driver/fast_math.f90
@@ -70,7 +70,7 @@
 ! UNSUPPORTED: system-windows
 ! UNSUPPORTED: target=powerpc{{.*}}
 ! RUN: %flang -ffast-math -### %s -o %t 2>&1 \
-! RUN:   --target=x86_64-unknown-linux -no-pie --gcc-toolchain="" \
+! RUN:   --target=x86_64-unknown-linux -no-pie \
 ! RUN:   --sysroot=%S/../../../clang/test/Driver/Inputs/basic_linux_tree \
 ! RUN: | FileCheck --check-prefix=CHECK-CRT %s
 ! CHECK-CRT: {{crtbegin.?\.o}}
Index: flang/test/Driver/driver-help.f90
===
--- flang/test/Driver/driver-help.f90
+++ flang/test/Driver/driver-help.f90
@@ -14,224 +14,242 @@
 ! HELP:USAGE: flang
 ! HELP-EMPTY:
 ! HELP-NEXT:OPTIONS:
-! HELP-NEXT: -###   Print (but do not run) the commands to run for this compilation
-! HELP-NEXT: -cpp   Enable predefined and command line preprocessor macros
-! HELP-NEXT: -c Only run preprocess, compile, and assemble steps
-! HELP-NEXT: -D = Define  to  (or 1 if  omitted)
-! HELP-NEXT: -emit-llvm Use the LLVM representation for assembler and object files
-! HELP-NEXT: -E Only run the preprocessor
+! HELP-NEXT: -###Print (but do not run) the commands to run for this compilation
+! HELP-NEXT: -cppEnable predefined and command line preprocessor macros
+! HELP-NEXT: -c  Only run preprocess, compile, and assemble steps
+! HELP-NEXT: -D =  Define  to  (or 1 if  omitted)
+! HELP-NEXT: -emit-llvm  Use the LLVM representation for assembler and object files
+! HELP-NEXT: -E  Only run the preprocessor
 ! HELP-NEXT: -falternative-parameter-statement
-! HELP-NEXT: Enable the old style PARAMETER statement
-! HELP-NEXT: -fapprox-func  Allow certain math function calls to be replaced with an approximately equivalent calculation
-! HELP-NEXT: -fbackslashSpecify that backslash in string introduces an escape character
-! 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
-! HELP-NEXT: -fdefault-integer-8Set the default integer and logical kind to an 8 byte wide type
-! HELP-NEXT: -fdefault-real-8   Set the default real kind to an 8 byte wide type
-! HELP-NEXT: -ffast-mathAllow aggressive, lossy floating-point optimizations
-! HELP-NEXT: -ffixed-form   Process source files in fixed form
+! HELP-NEXT: Enable the old style PARAMETER statement
+! HELP-NEXT: -fapprox-func   Allow certain math function calls to be replaced with an approximately equivalent calculation
+! HELP-NEXT: -fbackslash Specify that backslash in string introduces an escape character
+! HELP-NEXT: -fcolor-diagnostics Enable 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
+! HELP-NEXT: -fdefault-integer-8 Set the default integer and logical kind to an 8 byte wide type
+! HELP-NEXT: -fdefault-real-8Set the default real kind to an 8 byte wide type
+! HELP-NEXT: -ffast-math Allow aggressive, lossy floating-point optimizations
+! HELP-NEXT: -ffixed-formProcess source files in fixed form
 ! HELP-NEXT: -ffixed-line-length=
-! HELP-NEXT: Use  as character line width in fixed mode
-! HELP-NEXT: -ffp-contract= Form fused FP ops (e.g. FMAs)
-! HELP-NEXT: -ffree-formProcess source files in free form
-! HELP-NEXT: -fimplicit-noneNo implicit typing allowed unless overridden by IMPLICIT statements
+! HELP-NEXT: Use  as character line width in fixed mode
+! HELP-NEXT: -ffp-contract=   Form fused FP ops (e.g. FMAs)
+! HELP-NEXT: -ffree-form Process source files in free form
+! HELP-NEXT: -fhonor-infinities  Specify that floating-point optimizations are not allowed that assume arguments and results are not +-inf.
+! HELP-NEXT: -fhonor-nansSpecify that floating-point optimizations are not allowed that assume arguments and results are not NANs.
+! HELP-NEXT: 

[PATCH] D156320: [Flang][Driver] Add support for Rpass and related options

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

hey @victorkingi , I am still unsure about parsing these remarks options in two 
places:

- CompilerInvocation.cpp
- ExecuteCompilerInvocation.cpp

I think that it is important to clarify the relations between the two. In 
particular, it's normally the job of CompilerInvocaiton to make sure that e.g. 
`-Rpass -Rno-pass -Rpass` == `-Rpass` (so that `DiagnosticOptions::Remarks` 
only contains `-Rpass`). This might be tricky in practice if we want to support 
Regex, but would be good to document when e.g. populating 
`DiagnosticOptions::Remarks`.

I am also under the impression that extra complexity comes from the fact that 
this patch strives to support `-R` on top of 
`-R{no}pass`, `-R{no}pass-missed`, `-R{no}pass-analysis`. I also see some code 
left to support regex versions of the flags. Can you clean that up?




Comment at: flang/include/flang/Frontend/CodeGenOptions.h:76-81
+RK_Missing,// Remark argument not present on the command line.
+RK_Enabled,// Remark enabled via '-Rgroup'.
+RK_EnabledEverything,  // Remark enabled via '-Reverything'.
+RK_Disabled,   // Remark disabled via '-Rno-group'.
+RK_DisabledEverything, // Remark disabled via '-Rno-everything'.
+RK_WithPattern,// Remark pattern specified via '-Rgroup=regexp'.

I only see `RK_Enabled` and `RK_Disabled` being used, though I don't see 
`-Rgroup` nor `-Rno-group` being tested 樂 .



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:227
 
+  // Specifies, using a regex, which successful optimization passes done,
+  // to include in the final optimization record file generated. If not 
provided

Do you know whether that only includes middle-end, or also back-end passes?



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:240
+  // OptimizationRemark, OptimizationRemarkMissed and 
OptimizationRemarkAnalysis
+  // contain regex values which are used in optimizationRemarkHandler in
+  // FrontendActions.cpp to determine which remarks generated should be 
outputed

`optimizationRemarkHandler` is a member method of `DiagnosticHandler`, that you 
specialise in FrontendActions.cpp, right?



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:1034-1037
+  // Add the remark option requested i.e. pass, pass-missed or pass-analysis.
+  // This will be used later during processing warnings and remarks to give
+  // messages specific to a remark argument. That happens in
+  // processWarningOptions in ExecuteCompilerInvocation.cpp

How about:
```
Preserve all the remark options requested, e.g. -Rpass, -Rpass-missed or 
-Rpass-analysis. This will be used later when processing and outputting the 
remarks generated by LLVM in  ExecuteCompilerInvocation.cpp.
```



Comment at: flang/lib/Frontend/FrontendActions.cpp:976-1011
+  void
+  optimizationRemarkHandler(const llvm::DiagnosticInfoOptimizationBase ) {
+if (d.isPassed()) {
+  // Optimization remarks are active only if the -Rpass flag has a regular
+  // expression that matches the name of the pass name in \p d.
+  if (codeGenOpts.OptimizationRemark.patternMatches(d.getPassName()))
+emitOptimizationMessage(

victorkingi wrote:
> awarzynski wrote:
> > 
> The if statement still needs to return if the pattern doesn't match, should I 
> leave it the way it is?
Sorry, my bad, I missed that. Yeah, then leave it as is, but could you replace 
`const llvm::DiagnosticInfoOptimizationBase ` with something with more 
descriptive name? (I am referring to `d`)



Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:18
 #include "flang/Frontend/TextDiagnosticPrinter.h"
+#include "filesystem"
 #include "flang/Frontend/TextDiagnostic.h"

WOuld you be able to use 
https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Support/Path.h 
instead?



Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:21
+#include "string"
+#include "vector"
 #include "clang/Basic/DiagnosticOptions.h"

Would you be able to use 
https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ADT/SmallVector.h
 instead?



Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:23
 #include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "llvm/ADT/SmallString.h"

Is this needed?



Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:122-146
+static void processRemarkOptions(clang::DiagnosticsEngine ,
+ const clang::DiagnosticOptions ,
+ bool reportDiags = true) {
+  llvm::SmallVector _diags;
+  const llvm::IntrusiveRefCntPtr diagIDs =
+  diags.getDiagnosticIDs();
+

I am suggesting a few 

[PATCH] D156320: [Flang][Driver] Add support for Rpass and related options

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Some high level comments:

- The logic in `parseCodeGenArgs` in CompilerInvocation.cpp is a bit complex 
and quite specialized - could you move it to a dedicated method?
- In a fair few places this patch make references to "diagnostic flags" or 
"diagnostic options". That's borrowed from Clang where there's one code path 
for `-W` and `-R` flags. Note that in Clang the logic for `-W` 
is vastly more complex. This is completely absent in Flang. So, everything 
that's added here is added specifically for "remarks" and it would be helpful 
to be explicit about that.  For example, `processWarningOptions` is misleading. 
`processRemakrsOptions` would be more accurate.

Thank you :)




Comment at: flang/lib/Frontend/CompilerInvocation.cpp:227
 
+  // Specifies what passes to include. If not provided
+  // -fsave-optimization-record will include all passes.

> // Specifies what passes to include.

Could you be more specific, what passes are you referring to? Included where?



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:242
+
+  // Specify which missed passes should be reported using a regex.
+  opts.OptimizationRemarkMissed = parseOptimizationRemark(

>   // Specify which missed passes should be reported using a regex.

Reported where?



Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:37
+// Print any diagnostic option information to a raw_ostream.
+static void printDiagnosticOptions(llvm::raw_ostream ,
+   clang::DiagnosticsEngine::Level level,

victorkingi wrote:
> awarzynski wrote:
> > Why is this method needed and can it be tested?
> This method prints out the pass that was done e.g. [-Rpass=inline ], 
> [-Rpass=loop-delete] next to the optimization message.
> It is tested by the full remark message emitted test in 
> flang/test/Driver/optimization-remark.f90
Flang has very _very_ limited support for warning flags. So this is going to be 
used specifically for Remarks. Please update and document accordingly.



Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:30
 #include "llvm/Support/CommandLine.h"
+#include 
 

No longer needed, right? Also, please use the the format consistent with the 
other `#include`s.



Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:117
+
+void processWarningOptions(clang::DiagnosticsEngine ,
+   const clang::DiagnosticOptions ,

awarzynski wrote:
> ?
I find this method quite confusing.

1. It doesn't process warning options - it processes remarks options (so the 
name is inaccurate).
2. It deals with `-Rpass -Rno-pass` cases (i.e. postive + negative flag), but 
that's normally done in CompilerInvocation when parsing other options. See e.g. 
https://github.com/llvm/llvm-project/blob/c52d9509d40d3048914b144618232213e6076e05/flang/lib/Frontend/CompilerInvocation.cpp#L161-L163.
 Why not use that logic instead?
3. It's been extracted from Clang - it would be very helpful to note that and 
highlight the difference between this method and similar method in Clang.
4. You can safely trim it to only deal with Remarks (so e.g. update `const 
clang::DiagnosticOptions ,` in the signature)

Now, I am not requesting any major refactor here - I appreciate that e.g. these 
remark flags are defined a bit differently to other flags. But this method can 
be simplified and should be documented.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156320

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

[PATCH] D157151: [Driver] Refactor to use llvm Option's new Visibility flags

[Andrzej Warzynski via Phabricator via cfe-commits]

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

While quite extensive, I find the overall logic in this patch very well 
structured and executed in a very clean manner. It removes a lot of ambiguity, 
makes the overall design much easier to reason about and will definitely 
improve the overall health of the project. The benefits for LLVM Flang are 
almost immediate, see https://reviews.llvm.org/D157837. Thank you for 
implementing this, reviewing such patches is very satisfying and rewarding.

This might cause some disruption to downstream consumers of this API and 
Options.td. Hopefully, "update_options_td_flags.py" that you've included should 
minimise that. I suggest "advertising" it in the summary a bit more.

LGTM, great work, thank you!




Comment at: clang/lib/Driver/Driver.cpp:1941-1945
+  // TODO: We're overriding the mask for flang here to keep this NFC for the
+  // option refactoring, but what we really need to do is annotate the flags
+  // that Flang uses.
   if (IsFlangMode())
+VisibilityMask = llvm::opt::Visibility(options::FlangOption);

Note to myself - I should update this in https://reviews.llvm.org/D157837.



Comment at: flang/docs/FlangDriver.md:248-250
+Options that are also supported by clang should explicitly specify `Default` in
+`Vis`, and options that are only supported in Flang should not specify
+`Default`.

For consistency



Comment at: llvm/unittests/Option/OptionParsingTest.cpp:18
 
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+

Why not just update the test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157151

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

[PATCH] D156320: [Flang][Driver] Add support for Rpass and related options

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for the updates - this is looking really good! A few more suggestions 
and then I'll scan the whole thing again (sorry, there's been quite a lot of 
code going back and forth).




Comment at: flang/lib/Frontend/CompilerInvocation.cpp:1024
 
+  // add the remark option requested i.e. pass, pass-missed or pass-analysis.
+  // This will be used later during processing warnings and remarks to give





Comment at: flang/lib/Frontend/FrontendActions.cpp:976-1011
+  void
+  optimizationRemarkHandler(const llvm::DiagnosticInfoOptimizationBase ) {
+if (d.isPassed()) {
+  // Optimization remarks are active only if the -Rpass flag has a regular
+  // expression that matches the name of the pass name in \p d.
+  if (codeGenOpts.OptimizationRemark.patternMatches(d.getPassName()))
+emitOptimizationMessage(





Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:117
+
+void processWarningOptions(clang::DiagnosticsEngine ,
+   const clang::DiagnosticOptions ,

?



Comment at: flang/test/Driver/optimization-remark.f90:53
+end program forttest
\ No newline at end of file


FIXME


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156320

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

[PATCH] D157837: [flang][driver] Update the visibility of Clang options in Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

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

Add missing `TargetSpecific` flag to the definition of `mcpu_EQ`, remove 
redundant `TODO`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157837

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-help.f90
  flang/test/Driver/fast_math.f90

Index: flang/test/Driver/fast_math.f90
===
--- flang/test/Driver/fast_math.f90
+++ flang/test/Driver/fast_math.f90
@@ -70,7 +70,7 @@
 ! UNSUPPORTED: system-windows
 ! UNSUPPORTED: target=powerpc{{.*}}
 ! RUN: %flang -ffast-math -### %s -o %t 2>&1 \
-! RUN:   --target=x86_64-unknown-linux -no-pie --gcc-toolchain="" \
+! RUN:   --target=x86_64-unknown-linux -no-pie \
 ! RUN:   --sysroot=%S/../../../clang/test/Driver/Inputs/basic_linux_tree \
 ! RUN: | FileCheck --check-prefix=CHECK-CRT %s
 ! CHECK-CRT: {{crtbegin.?\.o}}
Index: flang/test/Driver/driver-help.f90
===
--- flang/test/Driver/driver-help.f90
+++ flang/test/Driver/driver-help.f90
@@ -14,224 +14,242 @@
 ! HELP:USAGE: flang
 ! HELP-EMPTY:
 ! HELP-NEXT:OPTIONS:
-! HELP-NEXT: -###   Print (but do not run) the commands to run for this compilation
-! HELP-NEXT: -cpp   Enable predefined and command line preprocessor macros
-! HELP-NEXT: -c Only run preprocess, compile, and assemble steps
-! HELP-NEXT: -D = Define  to  (or 1 if  omitted)
-! HELP-NEXT: -emit-llvm Use the LLVM representation for assembler and object files
-! HELP-NEXT: -E Only run the preprocessor
+! HELP-NEXT: -###Print (but do not run) the commands to run for this compilation
+! HELP-NEXT: -cppEnable predefined and command line preprocessor macros
+! HELP-NEXT: -c  Only run preprocess, compile, and assemble steps
+! HELP-NEXT: -D =  Define  to  (or 1 if  omitted)
+! HELP-NEXT: -emit-llvm  Use the LLVM representation for assembler and object files
+! HELP-NEXT: -E  Only run the preprocessor
 ! HELP-NEXT: -falternative-parameter-statement
-! HELP-NEXT: Enable the old style PARAMETER statement
-! HELP-NEXT: -fapprox-func  Allow certain math function calls to be replaced with an approximately equivalent calculation
-! HELP-NEXT: -fbackslashSpecify that backslash in string introduces an escape character
-! 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
-! HELP-NEXT: -fdefault-integer-8Set the default integer and logical kind to an 8 byte wide type
-! HELP-NEXT: -fdefault-real-8   Set the default real kind to an 8 byte wide type
-! HELP-NEXT: -ffast-mathAllow aggressive, lossy floating-point optimizations
-! HELP-NEXT: -ffixed-form   Process source files in fixed form
+! HELP-NEXT: Enable the old style PARAMETER statement
+! HELP-NEXT: -fapprox-func   Allow certain math function calls to be replaced with an approximately equivalent calculation
+! HELP-NEXT: -fbackslash Specify that backslash in string introduces an escape character
+! HELP-NEXT: -fcolor-diagnostics Enable 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
+! HELP-NEXT: -fdefault-integer-8 Set the default integer and logical kind to an 8 byte wide type
+! HELP-NEXT: -fdefault-real-8Set the default real kind to an 8 byte wide type
+! HELP-NEXT: -ffast-math Allow aggressive, lossy floating-point optimizations
+! HELP-NEXT: -ffixed-formProcess source files in fixed form
 ! HELP-NEXT: -ffixed-line-length=
-! HELP-NEXT: Use  as character line width in fixed mode
-! HELP-NEXT: -ffp-contract= Form fused FP ops (e.g. FMAs)
-! HELP-NEXT: -ffree-formProcess source files in free form
-! HELP-NEXT: -fimplicit-noneNo implicit typing allowed unless overridden by IMPLICIT statements
+! HELP-NEXT: Use  as character line width in fixed mode
+! HELP-NEXT: -ffp-contract=   Form fused FP ops (e.g. FMAs)
+! HELP-NEXT: -ffree-form Process source files in free form
+! HELP-NEXT: -fhonor-infinities  Specify that floating-point optimizations are not allowed that assume arguments and results are not +-inf.
+! HELP-NEXT: -fhonor-nansSpecify that floating-point optimizations are not 

[PATCH] D157837: [flang][driver] Update the visibility of Clang options in Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski marked 2 inline comments as done.
awarzynski added a comment.

Thank you all for reviewing!

In D157837#4584387 , @bogner wrote:

> I can't speak to which flags should be present in flang-new or not

That's determined by what's tested/used in tests.

> You'll need to update the patch to use the "ClangOption" spelling rather than 
> "Default" of course, as discussed in https://reviews.llvm.org/D157151

Yup, no problem. I may wait for your changes to land first.




Comment at: flang/test/Driver/target-cpu-features.f90:14
 ! Negative test. ARM cpu with x86 target.
-! RUN: not %flang --target=x86_64-linux-gnu -mcpu=cortex-a57 -c %s -### 2>&1 \
+! RUN: %flang --target=x86_64-linux-gnu -mcpu=cortex-a57 -c %s -### 2>&1 \
 ! RUN: | FileCheck %s -check-prefix=CHECK-NO-A57

tblah wrote:
> Why doesn't this fail now?
Missing `TargetSpecific` flag in the definition of the `mcpu_EQ` option. Sorry 
i didn't have time to debug earlier. To be fixed in the next revision.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157837

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

[PATCH] D157151: [Driver] Refactor to use llvm Option's new Visibility flags

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D157151#4582441 , @bogner wrote:

> This is a little bit complicated by the fact that the Option library is 
> already partially extracted out of clang (and used for other drivers, like 
> lld and lldb). The "Default" visibility is defined in llvm's Option library, 
> so calling it something like "Clang" would be pretty confusing for users that 
> aren't the clang Driver library.

Ah, I see. I guess `clangDriver` is a bit unique in the sense that there's 
quite a few drivers that rely on its Options.td:

- `clang,` `clang -cc1`, `clang -cc1as`, `clang-cl`,
- `flang-new`, `flang-new -fc1`.

> I suppose one option would be to throw something like `using ClangDriver = 
> llvm::Driver::Default;` in Options.h inside the `clang::driver::options` 
> namespace, and then using the alias in Options.td. Would that help?

That would make sense to me, yes. Otherwise the meaning of `Default` is 
unclear. These things tend to be obvious to folks familiar with the 
implementation. However, Options.td would normally be edited/updated by people 
less familiar with the fine details and more concerned about their favorite 
option and how it's implemented in their favorite tool.

> `Default` options are either options that don't specify `Vis` at all or 
> explicitly mention `Default`. They are not visible in `flang-new` after this 
> change unless they also specify `FlangOption`.

That's not quite true ATM. Although not used,  the following C++ option _is 
visible_ in `flang-new`:

  flang-new -fno-experimental-relative-c++-abi-vtables file.f90
  flang-new: warning: argument unused during compilation: 
'-fno-experimental-relative-c++-abi-vtables'

This can be tweaked in `getOptionVisibilityMask` (extracted from this patch):

  llvm::opt::Visibility
  Driver::getOptionVisibilityMask(bool UseDriverMode) const {
if (!UseDriverMode)
  return llvm::opt::Visibility(llvm::opt::Default);
if (IsCLMode())
  return llvm::opt::Visibility(options::CLOption);
if (IsDXCMode())
  return llvm::opt::Visibility(options::DXCOption);
if (IsFlangMode()) {
  // vvv HERE vvv
  // TODO: Does flang really want *all* of the clang driver options?
  // We probably need to annotate more specifically.
  return llvm::opt::Visibility(llvm::opt::Default | options::FlangOption);
}
return llvm::opt::Visibility(llvm::opt::Default);
  }

Now, prior to this change I couldn't find a way to disable unsupported Clang 
options in Flang. With this patch,

- all Clang options gain a visibility flag (atm that's `Default`),
- that flag can be used to disable options unsupported in Flang.

For this to work, all options supported by Flang need to have their visibility 
flags set accordingly. That's the goal, but I can see that we have missed quite 
a few options over the time (*). Updated here: https://reviews.llvm.org/D157837.

(*) There was no mechanism to enforce that. This is now changing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157151

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

[PATCH] D157837: [flang][driver] Update the visibility of Clang options in Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski created this revision.
awarzynski added a reviewer: bogner.
Herald added a reviewer: sscalpone.
Herald added projects: Flang, All.
awarzynski requested review of this revision.
Herald added subscribers: cfe-commits, wangpc, jplehr, sstefan1, jdoerfert, 
MaskRay.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.

Prior to D157151 , there was no mechanism to 
"disable" unsupported Clang
options in Flang. While the "help" text (`flang-new -help`) was indeed
configured not to display such options, the Flang compiler driver would
happily consume such options and only issue a warning when there was no
logic to parse it:

  flang-new -fno-experimental-relative-c++-abi-vtables file.f90
  flang-new: warning: argument unused during compilation: 
'-fno-experimental-relative-c++-abi-vtables'

D157151  introduces visibility flags. In 
particular, all Clang options
gain a visibility flag (`Default`) that can be excluded when in Flang
driver mode. This way the above invocation will lead to:

  flang-new: error: unknown argument 
'-fno-experimental-relative-c++-abi-vtables'; did you mean '-Xclang 
-fno-experimental-relative-c++-abi-vtables'?

This won't work unless all options/flags supported by Flang have their
visibility flags updated accordingly, hence the changes in Options.td.
Moving forward, this change will allow Flang better control over the
supported options.

Depends on D157151 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157837

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-help.f90
  flang/test/Driver/fast_math.f90
  flang/test/Driver/target-cpu-features.f90

Index: flang/test/Driver/target-cpu-features.f90
===
--- flang/test/Driver/target-cpu-features.f90
+++ flang/test/Driver/target-cpu-features.f90
@@ -11,7 +11,7 @@
 ! RUN: | FileCheck %s -check-prefix=CHECK-ARMV9
 
 ! Negative test. ARM cpu with x86 target.
-! RUN: not %flang --target=x86_64-linux-gnu -mcpu=cortex-a57 -c %s -### 2>&1 \
+! RUN: %flang --target=x86_64-linux-gnu -mcpu=cortex-a57 -c %s -### 2>&1 \
 ! RUN: | FileCheck %s -check-prefix=CHECK-NO-A57
 
 ! RUN: %flang --target=x86_64-linux-gnu -march=skylake -c %s -### 2>&1 \
Index: flang/test/Driver/fast_math.f90
===
--- flang/test/Driver/fast_math.f90
+++ flang/test/Driver/fast_math.f90
@@ -70,7 +70,7 @@
 ! UNSUPPORTED: system-windows
 ! UNSUPPORTED: target=powerpc{{.*}}
 ! RUN: %flang -ffast-math -### %s -o %t 2>&1 \
-! RUN:   --target=x86_64-unknown-linux -no-pie --gcc-toolchain="" \
+! RUN:   --target=x86_64-unknown-linux -no-pie \
 ! RUN:   --sysroot=%S/../../../clang/test/Driver/Inputs/basic_linux_tree \
 ! RUN: | FileCheck --check-prefix=CHECK-CRT %s
 ! CHECK-CRT: {{crtbegin.?\.o}}
Index: flang/test/Driver/driver-help.f90
===
--- flang/test/Driver/driver-help.f90
+++ flang/test/Driver/driver-help.f90
@@ -14,224 +14,242 @@
 ! HELP:USAGE: flang
 ! HELP-EMPTY:
 ! HELP-NEXT:OPTIONS:
-! HELP-NEXT: -###   Print (but do not run) the commands to run for this compilation
-! HELP-NEXT: -cpp   Enable predefined and command line preprocessor macros
-! HELP-NEXT: -c Only run preprocess, compile, and assemble steps
-! HELP-NEXT: -D = Define  to  (or 1 if  omitted)
-! HELP-NEXT: -emit-llvm Use the LLVM representation for assembler and object files
-! HELP-NEXT: -E Only run the preprocessor
+! HELP-NEXT: -###Print (but do not run) the commands to run for this compilation
+! HELP-NEXT: -cppEnable predefined and command line preprocessor macros
+! HELP-NEXT: -c  Only run preprocess, compile, and assemble steps
+! HELP-NEXT: -D =  Define  to  (or 1 if  omitted)
+! HELP-NEXT: -emit-llvm  Use the LLVM representation for assembler and object files
+! HELP-NEXT: -E  Only run the preprocessor
 ! HELP-NEXT: -falternative-parameter-statement
-! HELP-NEXT: Enable the old style PARAMETER statement
-! HELP-NEXT: -fapprox-func  Allow certain math function calls to be replaced with an approximately equivalent calculation
-! HELP-NEXT: -fbackslashSpecify that backslash in string introduces an escape character
-! 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
-! HELP-NEXT: -fdefault-integer-8Set the default integer and logical kind to an 8 byte wide type
-! 

[PATCH] D156320: [Flang][Driver] Add support for Rpass and related options

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for trimming this, it's much easier to review! A few more suggestions, 
but nothing major.




Comment at: flang/lib/Frontend/CompilerInvocation.cpp:227-263
+  bool needLocTracking = false;
+
+  if (!opts.OptRecordFile.empty())
+needLocTracking = true;
 
   if (const llvm::opt::Arg *a =
+  args.getLastArg(clang::driver::options::OPT_opt_record_passes)) {

```lang=cpp
  // coment
  if (const llvm::opt::Arg *a =
  args.getLastArg(clang::driver::options::OPT_opt_record_passes))
opts.OptRecordPasses = a->getValue();

  // coment
  if (const llvm::opt::Arg *a =
  args.getLastArg(clang::driver::options::OPT_opt_record_format))
opts.OptRecordFormat = a->getValue();

  // coment
  opts.OptimizationRemark = parseOptimizationRemark(
  diags, args, clang::driver::options::OPT_Rpass_EQ, "pass");

  // coment
  opts.OptimizationRemarkMissed = parseOptimizationRemark(
  diags, args, clang::driver::options::OPT_Rpass_missed_EQ, "pass-missed");

  // coment
  opts.OptimizationRemarkAnalysis = parseOptimizationRemark(
  diags, args, clang::driver::options::OPT_Rpass_analysis_EQ,
  "pass-analysis");

  if (opts.getDebugInfo() == llvm::codegenoptions::NoDebugInfo) {
// If the user requested a flag that requires source locations available in
// the backend, make sure that the backend tracks source location 
information.
bool needLocTracking = !opts.OptRecordFile.empty() || opts.OptRecordPasses 
||
   !opts.OptRecordFormat.empty() ||
   opts.OptimizationRemark.hasValidPattern() ||
   opts.OptimizationRemarkMissed.hasValidPattern() ||
   opts.OptimizationRemarkAnalysis.hasValidPattern();

if (needLocTracking)
  opts.setDebugInfo(llvm::codegenoptions::LocTrackingOnly);
  }
```

I might have made typos when editing this, but hopefully the overall logic 
makes sense. Basically, I am suggesting that "option parsing" is separated from 
the logic for setting up the location tracking in the backend.



Comment at: flang/lib/Frontend/FrontendActions.cpp:978-1005
+if (d.isPassed()) {
+  // Optimization remarks are active only if the -Rpass flag has a regular
+  // expression that matches the name of the pass name in \p D.
+  if (codeGenOpts.OptimizationRemark.patternMatches(d.getPassName()))
+emitOptimizationMessage(
+d, clang::diag::remark_fe_backend_optimization_remark);
+

https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code

```lang=cpp
  void
  optimizationRemarkHandler(const llvm::DiagnosticInfoOptimizationBase ) {
if (d.isPassed()) {
  // Optimization remarks are active only if the -Rpass flag has a regular
  // expression that matches the name of the pass name in \p D.
  if (codeGenOpts.OptimizationRemark.patternMatches(d.getPassName()))
emitOptimizationMessage(
d, clang::diag::remark_fe_backend_optimization_remark);
  return;
}

if (d.isMissed()) {
  // Missed optimization remarks are active only if the -Rpass-missed
  // flag has a regular expression that matches the name of the pass
  // name in \p D.
  if (codeGenOpts.OptimizationRemarkMissed.patternMatches(d.getPassName()))
emitOptimizationMessage(
d, clang::diag::remark_fe_backend_optimization_remark_missed);
  return;
}

assert(d.isAnalysis() && "Unknown remark type");

bool shouldAlwaysPrint = false;
if (auto *ora = llvm::dyn_cast())
  shouldAlwaysPrint = ora->shouldAlwaysPrint();

if (shouldAlwaysPrint ||
codeGenOpts.OptimizationRemarkAnalysis.patternMatches(
d.getPassName()))
  emitOptimizationMessage(
  d, clang::diag::remark_fe_backend_optimization_remark_analysis);
  }
```



Comment at: flang/lib/Frontend/FrontendActions.cpp:1007-1024
+  bool handleDiagnostics(const llvm::DiagnosticInfo ) override {
+switch (di.getKind()) {
+case llvm::DK_OptimizationRemark:
+  optimizationRemarkHandler(llvm::cast(di));
+  break;
+case llvm::DK_OptimizationRemarkMissed:
+  
optimizationRemarkHandler(llvm::cast(di));

Where is this method used?



Comment at: flang/lib/Frontend/FrontendActions.cpp:1136
 
+  BackendRemarkConsumer m(diags, codeGenOpts);
+

https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

> Variable names should be nouns (as they represent state). The name should be 
> camel case, and start with an upper case letter (e.g. Leader or Boats).



Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:169-171
+  clang::ProcessWarningOptions(flang->getDiagnostics(),
+   flang->getDiagnosticOpts());
+

[PATCH] D157151: [Driver] Refactor to use llvm Option's new Visibility flags

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

I've tested this locally and can confirm that the behavior of `clang` and 
`flang-new` has been preserved (as in, these changes won't be visible to the 
end users). Nice!

I think that it would be good to replace `Default` with e.g.

- `Clang`, or
- `ClangDriver`, or
- `ClangCompilerDriver`.

Or, at least, to make the meaning of `Default` much clearer (through e.g. 
comments). In general, I feel that `Default` is skewed towards this notion that 
`clang` (the compiler driver tool) is the main client of `clangDriver`. That 
used to be the case, but with Flang's driver also implemented in terms of 
`clangDriver` , that's no longer the case. Also, the long term goal is to 
extract the driver library out of Clang. One day :)

Also, note that `Default` options will be available in both `clang` and 
`flang-new`. That's the case today (so not something affected by your changes). 
Ideally, Flang should be able to disable those _Clang options_. That's not 
possible ATM. Contrary to Flang, Clang can disable _Flang options_ with 
`FlangOnlyOption`. There is no such flag for Flang ATM, but I think that we 
could re-use `Default` for that. WDYT?




Comment at: clang/docs/InternalsManual.rst:669-670
+  can affect how the option is treated or displayed.
+* ``Vis`` may contain visibility-specific "tags" associated with the option.
+  This lets us filter options for specific tools.
 * ``Alias`` denotes that the option is an alias of another option. This may be

IMHO, the difference between `Flags` and `Vis` is still unclear. Lets take this 
opportunity to refine this. IIUC:

* `vis` should be used to specify the drivers in which a particular option 
would be available. This attribute will impact `tool --help`,
* `flags`  can be used to limit the contexts in which a particular option/flag 
can be used (e.g. `NoXarchOption` or `LinkerOption`).



Comment at: clang/docs/InternalsManual.rst:682-685
+New options are recognized by the Clang driver if ``Vis`` is not specified or
+if it contains ``Default``. Options intended for the ``-cc1`` frontend must be
+explicitly marked with the ``CC1Option`` flag. Flags that specify ``CC1Option``
+but not ``Default`` will only be accessible via ``-cc1``.

"Clang driver" could mean two things:
* [[ 
https://github.com/llvm/llvm-project/blob/c52d9509d40d3048914b144618232213e6076e05/clang/lib/Driver/CMakeLists.txt#L17-L102
 | clangDriver ]] - the driver library shared between Clang and Flang,
* [[ 
https://github.com/llvm/llvm-project/blob/c52d9509d40d3048914b144618232213e6076e05/clang/tools/driver/CMakeLists.txt#L26-L36
 | clang ]] - Clang's compiler driver implemented in terms of `clangDriver` 
library.

I think that it's important to distinguish between the two in this document. In 
particular, the meaning of `Default` is confusing. Is the the default for the 
library (`clangDriver`) or the Clang compiler driver binary, `clang`. I believe 
it's the latter, but this wording suggests the former.

I appreciate that this wording pre-dates your patch (and probably Flang), but I 
think that it's worth taking this opportunity to refine this.



Comment at: llvm/docs/ReleaseNotes.rst:160
+* The ``Flags`` field of ``llvm::opt::Option`` has been split into ``Flags``
+  and ``Visibility`` to simplify option sharing between clang's drivers.
+  Overloads of ``llvm::opt::OptTable`` that use ``FlagsToInclude`` have been

[nit] Worth clarifying and advertising a bit more.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157151

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

[PATCH] D156320: [Flang][Driver] Add support for Rpass and related options

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:156-158
+/// Parse a remark command line argument. It may be missing, disabled/enabled 
by
+/// '-R[no-]group' or specified with a regular expression by '-Rgroup=regexp'.
+/// On top of that, it can be disabled/enabled globally by '-R[no-]everything'.

kiranchandramohan wrote:
> awarzynski wrote:
> > Could you give an example (and write a test ) for `-Rgroup=regexp`. Also, 
> > @kiranchandramohan , is this form actually needed? I am thinking that it's 
> > a complexity that could be avoided. It  could definitely simplify this 
> > patch.
> `Rpass=regex` is used, particularly `Rpass=.`. So this is required, but can 
> be deferred to a separate patch to simplify this one.
>  can be deferred to a separate patch to simplify this one

That would be a small win - the complexity comes from the fact that we can't 
rely on TableGen to define all possible options. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156320

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

[PATCH] D157151: [Driver] Refactor to use llvm Option's new Visibility flags

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Hey @bogner , I've only skimmed through so far and it's looking great! That 
Include/Exclude API was not fun to use. What you are proposing here takes 
Options.td to a much a better place - this is a much needed and long overdue 
refactor.

There's quite a bit of churn here, so I will need a few days to scan. In the 
meantime, could you update flang/docs/FlangDriver.md? And in general, please 
note that this updates (primarily) `clangDriver` logic, which is used by both 
by Clang and Flang. In particular, Options.td is shared. I think that it's 
worth highlighting that this change benefits both sub-projects.

> introduced in llvm Option

Could you add a link?




Comment at: clang/include/clang/Driver/Options.h:25-37
-  CoreOption = (1 << 8),
-  CLOption = (1 << 9),
-  CC1Option = (1 << 10),
-  CC1AsOption = (1 << 11),
-  NoDriverOption = (1 << 12),
-  LinkOption = (1 << 13),
-  FlangOption = (1 << 14),

What happens to `CoreOption`s? Same for `NoDriverOption`s?



Comment at: clang/include/clang/Driver/Options.td:193
 def m_x86_Features_Group : OptionGroup<"">,
-   Group, Flags<[CoreOption]>, DocName<"X86">;
+   Group, Vis<[Default, CLOption, DXCOption]>,
+   DocName<"X86">;

What's `Default` in `Vis<[Default, CLOption, DXCOption]>,`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157151

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

[PATCH] D156320: [Flang][Driver] Add support for Rpass and related options

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for the updates, more comments inline. Also:

> The remarks printed miss carets.

Why and can you share an example?

> The parseOptimizationRemark, addDiagnosticArgs and printDiagnosticOptions 
> functions created are identical to the ones used in Clang.

In which case we should seriously consider moving this code somewhere where it 
can be shared. If outside the scope of this change, please document in code 
that there's scope for re-use.




Comment at: flang/include/flang/Frontend/CodeGenOptions.h:72-118
+  enum class RemarkKind {
+RK_Missing,// Remark argument not present on the command line.
+RK_Enabled,// Remark enabled via '-Rgroup'.
+RK_EnabledEverything,  // Remark enabled via '-Reverything'.
+RK_Disabled,   // Remark disabled via '-Rno-group'.
+RK_DisabledEverything, // Remark disabled via '-Rno-everything'.
+RK_WithPattern,// Remark pattern specified via '-Rgroup=regexp'.

From what I can see, this has been borrowed almost verbatim from Clang: 
https://github.com/llvm/llvm-project/blob/3a100ea901ed79d6a06a5f018be2b4d3bbca51e8/clang/include/clang/Basic/CodeGenOptions.h#L331-L376.

This is fine (and common throughout the driver), but please document more. In 
particular:
* Highlight that ATM this code is identical that what Clang contains (and add a 
`TODO` to perhaps share with Clang at some point),
* Highlight that the list of options in Flang and Clang is _identical - it is 
really good that the interfaces in Clang and Flang are consistent. That's a 
good reason to re-use the logic from Clang.





Comment at: flang/lib/Frontend/CompilerInvocation.cpp:156-158
+/// Parse a remark command line argument. It may be missing, disabled/enabled 
by
+/// '-R[no-]group' or specified with a regular expression by '-Rgroup=regexp'.
+/// On top of that, it can be disabled/enabled globally by '-R[no-]everything'.

Could you give an example (and write a test ) for `-Rgroup=regexp`. Also, 
@kiranchandramohan , is this form actually needed? I am thinking that it's a 
complexity that could be avoided. It  could definitely simplify this patch.



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:786
   parseShowColorsArgs(args, /*defaultDiagColor=*/false);
+  res.getDiagnosticOpts().ShowColors = res.getFrontendOpts().showColors;
 

victorkingi wrote:
> Apparently without forwarding the color option to the CompilerInvocation, 
> flang doesn't print remark errors with color. Hence the need for this.
> Also, a question, why do we have 2 instances of DiagnosticsEngine, one in 
> CompilerInvocation and the other passed as an argument?
> Apparently without forwarding the color option to the CompilerInvocation, 
> flang doesn't print remark errors with color. Hence the need for this.

It is already "forwarded" here: 
https://github.com/llvm/llvm-project/blob/3a100ea901ed79d6a06a5f018be2b4d3bbca51e8/flang/lib/Frontend/CompilerInvocation.cpp#L117-L122.
 Could you explain why you need this change _here_?

> Also, a question, why do we have 2 instances of DiagnosticsEngine, one in 
> CompilerInvocation and the other passed as an argument?

[[ 
https://github.com/llvm/llvm-project/blob/3a100ea901ed79d6a06a5f018be2b4d3bbca51e8/flang/tools/flang-driver/fc1_main.cpp#L37-L40
 | One  ]] is for the driver itself, to generate diagnostics related to "driver 
errors" (e.g. option parsing errors). The [[ 
https://github.com/llvm/llvm-project/blob/3a100ea901ed79d6a06a5f018be2b4d3bbca51e8/flang/include/flang/Frontend/CompilerInstance.h#L64-L65
 | other ]] belongs to `CompilerInstance` rather than `CompilerInvocation` and 
is used to report compilation errors (e.g. semantic or parsing errors). 



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:1070
   }
 
+  addDiagnosticArgs(args, clang::driver::options::OPT_R_Group,

Is this specifically for parsing remarks options? Please add a comment



Comment at: flang/lib/Frontend/FrontendActions.cpp:927
 
+class StandaloneBackendConsumer : public llvm::DiagnosticHandler {
+

Why `StandaloneBackendConsumer`? Isn't this specifically for remarks? Also, 
could you document this class a bit?



Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:37
+// Print any diagnostic option information to a raw_ostream.
+static void printDiagnosticOptions(llvm::raw_ostream ,
+   clang::DiagnosticsEngine::Level level,

Why is this method needed and can it be tested?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156320

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

[PATCH] D157410: [Flang][Driver] Enable Rpass and other R family options.

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski accepted this revision.
awarzynski added a comment.

Thanks! LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157410

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

[PATCH] D156320: [Flang][Driver] Add support for Rpass and related options

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:173-174
+if (!result.Regex->isValid(regexError)) {
+  diags.Report(clang::diag::err_drv_optimization_remark_pattern)
+  << regexError << a->getAsString(args);
+  return false;

Could you add a test to exercise this diagnostic?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156320

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

[PATCH] D157410: [Flang][Driver] Enable Rpass and other R family options.

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski accepted this revision.
awarzynski added a comment.

LG, with a small suggestion :) (in the spirit of cleaning up Options.td)




Comment at: clang/include/clang/Driver/Options.td:819-831
+def Rpass_EQ : Joined<["-"], "Rpass=">, Group, 
Flags<[CC1Option, FlangOption, FC1Option]>,
   HelpText<"Report transformations performed by optimization passes whose "
"name matches the given POSIX regular expression">;
 def Rpass_missed_EQ : Joined<["-"], "Rpass-missed=">, Group,
-  Flags<[CC1Option]>,
+  Flags<[CC1Option, FlangOption, FC1Option]>,
   HelpText<"Report missed transformations by optimization passes whose "
"name matches the given POSIX regular expression">;

IIUC, these are the all options for optimisation remarks? I would extract them 
to a separate "block" and wrap like this:
```
//===--===//
// Optimisation remark options
//===--===//
let Flags = [CC1Option, FC1Option, FlangOption] in {

}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157410

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

[PATCH] D156320: [FLang] Add support for Rpass flag

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Hey @victorkingi , thank you for working on this :)

There's quite a lot going on here and I am thinking that it might be good to 
split this into a few patches? Also, please note that Flang's driver, unlike 
Clang, uses MLIR's coding style (use `camelCase` instead of `CamelCase`).

> A StandaloneBackendConsumer was implemented but the DiagnosticsEngine still 
> doesn't print to console the remarks produced.

This could mean that it's not being deconstructed correctly. Or that it 
requires explicit flushing. Can you verify that it contains the expected 
remarks?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156320

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

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

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a subscriber: everythingfunctional.
awarzynski added a comment.

In D125788#4556324 , @MaskRay wrote:

> If you continue this work, consider landing this rename in multiple phases, 
> e.g.
>
> - Use lit substitutions instead of `flang-new` and other preparatory stuff so 
> that the next step has very little change
> - change flang-new to flang and add flang-new as a symlink for compatibility
> - remove the symlink `flang-new`, when it is no longer used.
>
> Many build bots can fail in weird ways and ensuring that we will not revert a 
> large change prevents causing churn to the tests.

Thank you, these are great pointers! CC @everythingfunctional


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125788

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

[PATCH] D156524: [flang][nfc] Simplify option forwarding

[Andrzej Warzynski via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe88ff8a79b8b: [flang][nfc] Simplify option forwarding 
(authored by awarzynski).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156524

Files:
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/test/Driver/frontend-forwarding.f90


Index: flang/test/Driver/frontend-forwarding.f90
===
--- flang/test/Driver/frontend-forwarding.f90
+++ flang/test/Driver/frontend-forwarding.f90
@@ -17,6 +17,7 @@
 ! RUN: -fpass-plugin=Bye%pluginext \
 ! RUN: -fversion-loops-for-stride \
 ! RUN: -flang-experimental-polymorphism \
+! RUN: -flang-experimental-hlfir \
 ! RUN: -mllvm -print-before-all \
 ! RUN: -save-temps=obj \
 ! RUN: -P \
@@ -36,7 +37,8 @@
 ! CHECK: "-freciprocal-math"
 ! CHECK: "-fconvert=little-endian"
 ! CHECK: "-fpass-plugin=Bye
-! CHECK: "-flang-experimental-polymorphism"
 ! CHECK: "-fversion-loops-for-stride"
+! CHECK: "-flang-experimental-polymorphism"
+! CHECK: "-flang-experimental-hlfir"
 ! CHECK: "-mllvm" "-print-before-all"
 ! CHECK: "-save-temps=obj"
Index: clang/lib/Driver/ToolChains/Flang.cpp
===
--- clang/lib/Driver/ToolChains/Flang.cpp
+++ clang/lib/Driver/ToolChains/Flang.cpp
@@ -140,12 +140,11 @@
   !stackArrays->getOption().matches(options::OPT_fno_stack_arrays))
 CmdArgs.push_back("-fstack-arrays");
 
-  if (Args.hasArg(options::OPT_flang_experimental_hlfir))
-CmdArgs.push_back("-flang-experimental-hlfir");
-  if (Args.hasArg(options::OPT_flang_experimental_polymorphism))
-CmdArgs.push_back("-flang-experimental-polymorphism");
   if (shouldLoopVersion(Args))
 CmdArgs.push_back("-fversion-loops-for-stride");
+
+  Args.AddAllArgs(CmdArgs, {options::OPT_flang_experimental_hlfir,
+options::OPT_flang_experimental_polymorphism});
 }
 
 void Flang::addPicOptions(const ArgList , ArgStringList ) const {


Index: flang/test/Driver/frontend-forwarding.f90
===
--- flang/test/Driver/frontend-forwarding.f90
+++ flang/test/Driver/frontend-forwarding.f90
@@ -17,6 +17,7 @@
 ! RUN: -fpass-plugin=Bye%pluginext \
 ! RUN: -fversion-loops-for-stride \
 ! RUN: -flang-experimental-polymorphism \
+! RUN: -flang-experimental-hlfir \
 ! RUN: -mllvm -print-before-all \
 ! RUN: -save-temps=obj \
 ! RUN: -P \
@@ -36,7 +37,8 @@
 ! CHECK: "-freciprocal-math"
 ! CHECK: "-fconvert=little-endian"
 ! CHECK: "-fpass-plugin=Bye
-! CHECK: "-flang-experimental-polymorphism"
 ! CHECK: "-fversion-loops-for-stride"
+! CHECK: "-flang-experimental-polymorphism"
+! CHECK: "-flang-experimental-hlfir"
 ! CHECK: "-mllvm" "-print-before-all"
 ! CHECK: "-save-temps=obj"
Index: clang/lib/Driver/ToolChains/Flang.cpp
===
--- clang/lib/Driver/ToolChains/Flang.cpp
+++ clang/lib/Driver/ToolChains/Flang.cpp
@@ -140,12 +140,11 @@
   !stackArrays->getOption().matches(options::OPT_fno_stack_arrays))
 CmdArgs.push_back("-fstack-arrays");
 
-  if (Args.hasArg(options::OPT_flang_experimental_hlfir))
-CmdArgs.push_back("-flang-experimental-hlfir");
-  if (Args.hasArg(options::OPT_flang_experimental_polymorphism))
-CmdArgs.push_back("-flang-experimental-polymorphism");
   if (shouldLoopVersion(Args))
 CmdArgs.push_back("-fversion-loops-for-stride");
+
+  Args.AddAllArgs(CmdArgs, {options::OPT_flang_experimental_hlfir,
+options::OPT_flang_experimental_polymorphism});
 }
 
 void Flang::addPicOptions(const ArgList , ArgStringList ) const {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156524: [flang][nfc] Simplify option forwarding

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for taking a look!




Comment at: clang/lib/Driver/ToolChains/Flang.cpp:146-147
+
+  Args.AddAllArgs(CmdArgs, {options::OPT_flang_experimental_hlfir,
+options::OPT_flang_experimental_polymorphism});
 }

kiranchandramohan wrote:
> Is the order changing in the tests below (poly before hlfir) expected?
Yes. `AddAllArgs` will consume `-flang-experimental-polymorphism` first because:
* it satisfies the condition specified here (i.e. either 
`-flang-experimental-hlfir` or `-flang-experimental-polymorphism` ), and
* that's the first option it encounters (i.e. it is specified before 
`-flang-experimental-hlfir` in the test file).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156524

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

[PATCH] D156524: [flang][nfc] Simplify option forwarding

[Andrzej Warzynski via Phabricator via cfe-commits]

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

Use `AddAllArgs` to keep the implementation succinct. Also adds missing
'-flang-experimental-hlfir` in "frontend-forwarding.f90"


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156524

Files:
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/test/Driver/frontend-forwarding.f90


Index: flang/test/Driver/frontend-forwarding.f90
===
--- flang/test/Driver/frontend-forwarding.f90
+++ flang/test/Driver/frontend-forwarding.f90
@@ -17,6 +17,7 @@
 ! RUN: -fpass-plugin=Bye%pluginext \
 ! RUN: -fversion-loops-for-stride \
 ! RUN: -flang-experimental-polymorphism \
+! RUN: -flang-experimental-hlfir \
 ! RUN: -mllvm -print-before-all \
 ! RUN: -save-temps=obj \
 ! RUN: -P \
@@ -36,7 +37,8 @@
 ! CHECK: "-freciprocal-math"
 ! CHECK: "-fconvert=little-endian"
 ! CHECK: "-fpass-plugin=Bye
-! CHECK: "-flang-experimental-polymorphism"
 ! CHECK: "-fversion-loops-for-stride"
+! CHECK: "-flang-experimental-polymorphism"
+! CHECK: "-flang-experimental-hlfir"
 ! CHECK: "-mllvm" "-print-before-all"
 ! CHECK: "-save-temps=obj"
Index: clang/lib/Driver/ToolChains/Flang.cpp
===
--- clang/lib/Driver/ToolChains/Flang.cpp
+++ clang/lib/Driver/ToolChains/Flang.cpp
@@ -140,12 +140,11 @@
   !stackArrays->getOption().matches(options::OPT_fno_stack_arrays))
 CmdArgs.push_back("-fstack-arrays");
 
-  if (Args.hasArg(options::OPT_flang_experimental_hlfir))
-CmdArgs.push_back("-flang-experimental-hlfir");
-  if (Args.hasArg(options::OPT_flang_experimental_polymorphism))
-CmdArgs.push_back("-flang-experimental-polymorphism");
   if (shouldLoopVersion(Args))
 CmdArgs.push_back("-fversion-loops-for-stride");
+
+  Args.AddAllArgs(CmdArgs, {options::OPT_flang_experimental_hlfir,
+options::OPT_flang_experimental_polymorphism});
 }
 
 void Flang::addPicOptions(const ArgList , ArgStringList ) const {


Index: flang/test/Driver/frontend-forwarding.f90
===
--- flang/test/Driver/frontend-forwarding.f90
+++ flang/test/Driver/frontend-forwarding.f90
@@ -17,6 +17,7 @@
 ! RUN: -fpass-plugin=Bye%pluginext \
 ! RUN: -fversion-loops-for-stride \
 ! RUN: -flang-experimental-polymorphism \
+! RUN: -flang-experimental-hlfir \
 ! RUN: -mllvm -print-before-all \
 ! RUN: -save-temps=obj \
 ! RUN: -P \
@@ -36,7 +37,8 @@
 ! CHECK: "-freciprocal-math"
 ! CHECK: "-fconvert=little-endian"
 ! CHECK: "-fpass-plugin=Bye
-! CHECK: "-flang-experimental-polymorphism"
 ! CHECK: "-fversion-loops-for-stride"
+! CHECK: "-flang-experimental-polymorphism"
+! CHECK: "-flang-experimental-hlfir"
 ! CHECK: "-mllvm" "-print-before-all"
 ! CHECK: "-save-temps=obj"
Index: clang/lib/Driver/ToolChains/Flang.cpp
===
--- clang/lib/Driver/ToolChains/Flang.cpp
+++ clang/lib/Driver/ToolChains/Flang.cpp
@@ -140,12 +140,11 @@
   !stackArrays->getOption().matches(options::OPT_fno_stack_arrays))
 CmdArgs.push_back("-fstack-arrays");
 
-  if (Args.hasArg(options::OPT_flang_experimental_hlfir))
-CmdArgs.push_back("-flang-experimental-hlfir");
-  if (Args.hasArg(options::OPT_flang_experimental_polymorphism))
-CmdArgs.push_back("-flang-experimental-polymorphism");
   if (shouldLoopVersion(Args))
 CmdArgs.push_back("-fversion-loops-for-stride");
+
+  Args.AddAllArgs(CmdArgs, {options::OPT_flang_experimental_hlfir,
+options::OPT_flang_experimental_polymorphism});
 }
 
 void Flang::addPicOptions(const ArgList , ArgStringList ) const {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155452: [Flang] Add support for fsave-optimization-record

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski accepted this revision.
awarzynski added a comment.

LGTM, thanks for the updates! (please address my comment in 
"frontend-forwarding.f90" before landing this)




Comment at: flang/test/Driver/frontend-forwarding.f90:26
 
+! RUN: %flang -### %s 2>&1 \
+! RUN: -foptimization-record-file=%t.opt.yaml \

victorkingi wrote:
> awarzynski wrote:
> > Is a dedicated driver invocation needed for this test?
> since -fsave-optimization-record and foptimization-record-file both produce 
> opt-record-file and opt-record-format flags, I couldn't find an easier way to 
> test for both calls without having to use 2 separate flang-new invocations.
> 
> We could ignore the foptimization-record-file test and assume if 
> fsave-optimization-record passes, then the former passes as well
Ah, I see what's happening here. Neither `-fsave-optimization-record ` nor 
`-foptimization-record-file` are "forwarded" from `flang-new` to `flang-new 
-fc1` (*). So, shouldn't really be tested in this file. Please remove this 
change and I'll update the comment in this file to clarify this for our future 
selves. Apologies for the confusion.

(*) Forwarding in this context means "passing verbatim from `flang-new` to 
`flang-new -fc1`"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155452

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

[PATCH] D155452: [Flang] Add support for fsave-optimization-record

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for working on this, LG! I've left a few minor comments inline.




Comment at: clang/include/clang/Driver/Options.td:6449-6455
+def opt_record_file : Separate<["-"], "opt-record-file">, Flags<[FC1Option, 
CC1Option]>,
+  HelpText<"File name to use for YAML optimization record output">,
+  MarshallingInfoString>;
+def opt_record_passes : Separate<["-"], "opt-record-passes">, 
Flags<[FC1Option, CC1Option]>,
+  HelpText<"Only record remark information for passes whose names match the 
given regular expression">;
+def opt_record_format : Separate<["-"], "opt-record-format">, 
Flags<[FC1Option, CC1Option]>,
+  HelpText<"The format used for serializing remarks (default: YAML)">;

Currently, these options have the following flags set:
*  `CC1Option` and `NoDriverOption`.
See:
* 
https://github.com/llvm/llvm-project/blob/5da317a79e3e53f17c6d356361807df1d16a0b66/clang/include/clang/Driver/Options.td#L6156

IIUC, you'd like this to be CC1Option` and `NoDriverOption` _and_ `FC1Option`. 
You can do that by moving these definitions here:
* 
https://github.com/llvm/llvm-project/blob/5da317a79e3e53f17c6d356361807df1d16a0b66/clang/include/clang/Driver/Options.td#L6810-L6835



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:195-197
+  args.getLastArg(clang::driver::options::OPT_opt_record_file)) {
+opts.OptRecordFile = a->getValue();
+  }

Drop braces: 
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements.
 Same for the other options below.

Btw, I'd be OK with moving this to a dedicated hook for parsing e.g. "opt 
remark options". 



Comment at: flang/lib/Frontend/FrontendActions.cpp:961-965
+  clang::DiagnosticsEngine  = ci.getDiagnostics();
+  const CodeGenOptions  = ci.getInvocation().getCodeGenOpts();
+  Fortran::lower::LoweringOptions  =
+  ci.getInvocation().getLoweringOpts();
+

victorkingi wrote:
> Thought this refactoring might help making the function clearer. Let me know 
> if its unnecessary
It's a nice clean-up!

I'd move it to a separate patch, but the noise level is low, so I am not too 
concerned. 



Comment at: flang/test/Driver/frontend-forwarding.f90:26
 
+! RUN: %flang -### %s 2>&1 \
+! RUN: -foptimization-record-file=%t.opt.yaml \

Is a dedicated driver invocation needed for this test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155452

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

[PATCH] D154869: [Flang] [FlangRT] Implement FlangRT library as solution to Flang's runtime LLVM integration

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:934-936
+  // Default to the /../lib and
+  // /../runtimes/runtimes-bins/lib directories. This works fine
+  // on the platforms that we have tested so far. We will probably have to

pscoro wrote:
> awarzynski wrote:
> > Am I correct thinking that:
> > * "/../lib" is for `Fortran_main.a`, and
> > * "/../runtimes/runtimes-bins/lib " is for `libflang-rt`?
> > 
> > Could you document this? Thanks!
> Yep thats right, documentation added
> Yep thats right, documentation added

Could you document _where_ these libraries are located? Also, could you add 
some relevant comments here? For example:
```
  // Add search path for XYZ
  llvm::sys::path::append(BuildLibPath, "lib");
```



Comment at: flang/docs/Flang-rt.md:15-16
+```
+The flang driver requires two libraries for linking into any user executable,
+`Fortran_main.a` and Flang-rt. 
+## Motivation

This statement is not quite true - the driver can do a lot without requiring 
these libraries. I suspect that you wanted o say that generating executables 
requires these libraries.

[nit] flang dirver --> Flang driver



Comment at: flang/docs/Flang-rt.md:17
+`Fortran_main.a` and Flang-rt. 
+## Motivation
+Before Flang-rt, the driver would need to link Fortran_main, FortranRuntime,

Motivation for ...? It's not clear what the following section aims to achieve.

I think that it would be good to take a step beck and decide who this 
documented is intended for. If it's the end-user then, imho, this section 
doesn't belong here. In documentation like this I'd focus on "what" and "how" 
as opposed to "why". 

Even if this document is intended for developers, I wouldn't focus too much on 
the original motivations for this change and instead explain the design and its 
benefits:
* possibility to build the runtime independently of Flang (what's the benefit 
of that?)
* CMake set-up that's consistent with other sub-projects in LLVM,
* a convenient wrapper for multiple (i.e. 2) Flang runtime libraries,
* automated logic to build shared and static version of the runtime libs (to 
enable supporting e.g. `-static`?).

I hope that this makes sense.



Comment at: flang/docs/Flang-rt.md:30-31
+## Fortran_main
+Fortran_main is left out of Flang-rt because it is required to be linked
+statically. The link type of Flang-rt can be configured with a CMake option.
+

Could you document "why"?



Comment at: flang/docs/Flang-rt.md:35
+In order to decouple the common dependency between compiler and runtime,
+FortranDecimal's sources get built a second time into a library target named
+FortranDecimalRT. FortranDecimal is built for the compiler and FortranDecimalRT

Perhaps turn this into a link to the CMake definition?



Comment at: flang/docs/Flang-rt.md:61-63
+The two build paths mentioned above get implicitly added as library paths at 
the
+invocation of the driver. If Flang-rt is a shared library, you must add its 
path
+to the environment variable `LD_LIBRARY_PATH`.

1. Please add an example.
2. "you must add its path to the environment variable `LD_LIBRARY_PATH`" <-- 
this is just one way to provide a search path for the dynamic linker. There are 
other ways too. It would be good to clarify that a) one has to make the dynamic 
linker aware _where_ to look and that b) `LD_LIBRARY_PATH` is one such 
mechanism that can be used. But I would be careful not to "endorse" it - 
mishandling `LD_LIBRARY_PATH` can lead to tricky surprises.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154869

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

[PATCH] D154869: [Flang] [FlangRT] Implement FlangRT library as solution to Flang's runtime LLVM integration

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:934-936
+  // Default to the /../lib and
+  // /../runtimes/runtimes-bins/lib directories. This works fine
+  // on the platforms that we have tested so far. We will probably have to

Am I correct thinking that:
* "/../lib" is for `Fortran_main.a`, and
* "/../runtimes/runtimes-bins/lib " is for `libflang-rt`?

Could you document this? Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154869

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

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

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/test/Driver/pic-flags.f90:3
 
-! RUN: %flang -### %s --target=aarch64-linux-gnu 2>&1 | FileCheck %s 
--check-prefix=CHECK-NOPIE
-! RUN: %flang -### %s --target=aarch64-linux-gnu -fno-pie 2>&1 | FileCheck %s 
--check-prefix=CHECK-NOPIE
+! RUN: %flang -v -S -emit-llvm -o - %s --target=aarch64-linux-gnu 2>&1 | 
FileCheck %s --check-prefixes=CHECK,CHECK-PIE-LEVEL2,CHECK-PIE-LEVEL2-IR
+! RUN: %flang -v -S -emit-llvm -o - %s --target=aarch64-linux-gnu -fpie 2>&1 | 
FileCheck %s --check-prefixes=CHECK,CHECK-PIE-LEVEL1,CHECK-PIE-LEVEL1-IR

clementval wrote:
> This test has been failing on my side for very long time. Just curious why we 
> check for PIE-LEVEL-2 when no level is given. Where is the default given?
> 
> On my machine I have `-mrelocation-model static` for this run line. 
> This test has been failing on my side for very long time. 

If this is failing for you then it should also fail on one of the Flang 
buildbots, right? Do you know what's different/special in your configuration?

> Where is the default given?

You want to check:
* [[ 
https://github.com/llvm/llvm-project/blob/2712b2805b47f10b3864ab19a4016ea306126ad7/clang/lib/Driver/ToolChains/Flang.cpp#L149-L168
 | Flang::addPicOption ]] (Flang.cpp)
* [[ 
https://github.com/llvm/llvm-project/blob/2712b2805b47f10b3864ab19a4016ea306126ad7/clang/lib/Driver/ToolChains/CommonArgs.cpp#L1399
 | ParsePICArgs ]] (CommonArgs.cpp)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131533

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

[PATCH] D153379: Remove -flang-experimental-exec flag

[Andrzej Warzynski via Phabricator via cfe-commits]

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

This is in line with the conclusions of 
https://discourse.llvm.org/t/proposal-rename-flang-new-to-flang/:

> We should remove the need for the -flang-experimental-exec flag. There’s no 
> need to hide things to this level given the evolving maturity of LLVM’s 
> flang. This will be an immediate improvement for all flang-new users.

LGTM!

Thanks a ton @everythingfunctional , great to see this flag finally being 
removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153379

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

[PATCH] D153281: [flang] add -flang-experimental-polymorphism flag to flang-new

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski accepted this revision.
awarzynski added a comment.

LGTM, thanks David!

Could you also add a test in 
https://github.com/llvm/llvm-project/blob/main/flang/test/Driver/frontend-forwarding.f90?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153281

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

[PATCH] D151088: [flang][hlfir] Separate -emit-fir and -emit-hlfir for flang-new

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski accepted this revision.
awarzynski added a comment.

Thanks for the updates, LGTM!




Comment at: flang/test/HLFIR/flang-experimental-hlfir-flag.f90:7-12
+! | Action  | -flang-experimental-hlfir? | Result  
|
+! | === | == | === 
|
+! | -emit-hlfir | N  | Outputs HLFIR   
|
+! | -emit-hlfir | Y  | Outputs HLFIR   
|
+! | -emit-fir   | N  | Outputs FIR, using old lowering 
|
+! | -emit-fir   | Y  | Outputs FIR, lowering via HLFIR 
|

This comment is pure gold!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151088

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

[PATCH] D151088: [flang][hlfir] Separate -emit-fir and -emit-hlfir for flang-new

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/lib/Frontend/FrontendActions.cpp:651
+// Lower using HLFIR then run the FIR to HLFIR pipeline
+void CodeGenAction::lowerHLFIRToFIR() {
+  assert(mlirModule && "The MLIR module has not been generated yet.");

tblah wrote:
> awarzynski wrote:
> > I wouldn't really consider this hook as a separate action. Instead, I'd use 
> > it here: 
> > https://github.com/llvm/llvm-project/blob/6130c9df99a7a7eb9c6adc118a48f8f2acc534ab/flang/lib/Frontend/FrontendActions.cpp#L917-L920.
> >  As in, it basically "tweaks" `EmitMLIR` (which I would rename as 
> > `EmitFIR`).
> This is very different to `EmitMLIR`.
> 
> `EmitMLIR` will print the MLIR produced by lowering (HLFIR or FIR depending 
> upon the `-flang-experimental-hlfir` flag).
> 
> This action will run lowering, always generating HLFIR. Then it will run the 
> HLFIR pass pipeline to lower the HLFIR into FIR, and output that FIR (which 
> will be very different to FIR generated directly from lowering without going 
> through HLFIR).
So, at the moment:
* `EmitMLIR` will generate either FIR of HLFIR,
* `lowerHLFIRToFIR` will also generate FIR via HFLIR.

When `-flang-experimental-hlfir` is used, both actions do the same thing 
(**what**) - generate FIR. The difference becomes in **how** this is achieved.  
However, actions represent "what", not "how" and hence my suggestion. In fact,  
this would be totally valid:
```
  /// (with `-flang-experimental-hlfir`) Lower to FIR and print
  EmitMLIR,

  /// Lower to FIR and print, go via HLFIR
  EmitHLFIRToFIR,
```
With `EmitFIR` and `EmitHLFIR` (instead of `EmitMLIR` and `EmitHLFIRToFIR`), 
you'd preserve all the functionality and also make sure that there's never 
overlap between actions, right? Additionally, you'd still have 
`-flang-experimental-hlfir` that would tweak `EmitFIR`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151088

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

[PATCH] D151088: [flang][hlfir] Separate -emit-fir and -emit-hlfir for flang-new

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks Tom, mostly makes sense, but I suggest the following:

- `EmitMLIR` --> `EmitFIR` (nfc, just clarifying the name),
- `EmitHLFIRToFIR` --> `EmitHLFIR` (functional change).

More comments inline.




Comment at: flang/include/flang/Frontend/FrontendOptions.h:42
+  /// resulting FIR
+  EmitHLFIRToFIR,
+

To me having `EmitFIR` and `EmitHLFIR` would make more sense. With 2 dialects, 
`EmitMLIR` becomes rather confusing (and, I suspect, rarely used).



Comment at: flang/lib/Frontend/FrontendActions.cpp:651
+// Lower using HLFIR then run the FIR to HLFIR pipeline
+void CodeGenAction::lowerHLFIRToFIR() {
+  assert(mlirModule && "The MLIR module has not been generated yet.");

I wouldn't really consider this hook as a separate action. Instead, I'd use it 
here: 
https://github.com/llvm/llvm-project/blob/6130c9df99a7a7eb9c6adc118a48f8f2acc534ab/flang/lib/Frontend/FrontendActions.cpp#L917-L920.
 As in, it basically "tweaks" `EmitMLIR` (which I would rename as `EmitFIR`).



Comment at: flang/lib/Frontend/FrontendActions.cpp:652
+void CodeGenAction::lowerHLFIRToFIR() {
+  assert(mlirModule && "The MLIR module has not been generated yet.");
+

This `mlirModule` comes from here: 
https://github.com/llvm/llvm-project/blob/6130c9df99a7a7eb9c6adc118a48f8f2acc534ab/flang/lib/Frontend/FrontendActions.cpp#L277.
 That will either be FIR or HLFIR, depending on whether 
`-flang-experimental-hlfir` was used or not, right?



Comment at: flang/lib/Frontend/FrontendActions.cpp:673
+unsigned diagID = ci.getDiagnostics().getCustomDiagID(
+clang::DiagnosticsEngine::Error, "Lowering to LLVM IR failed");
+ci.getDiagnostics().Report(diagID);

"Lowering to LLVM IR"? ;-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151088

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

[PATCH] D150354: [OpenMP][MLIR][Flang][bbc][Driver] Add fopenmp-version and generate corresponding MLIR attribute

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

All in all LGTM, but I'm not sure whether Flang should be defaulting to OpenMP 
5.0. AFAIK, that's not supported yet.




Comment at: clang/lib/Driver/ToolChains/Flang.cpp:34
+  Args.AddAllArgs(CmdArgs, {options::OPT_ffixed_form,
+options::OPT_ffree_form,
+options::OPT_ffixed_line_length_EQ,

Is this clang-format? Looks like a big block of unrelated changes (not against 
it).


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

https://reviews.llvm.org/D150354

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

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

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D134821#4317371 , @peixin wrote:

>> @peixin , wdyt?
>
> @awarzynski Hi Andrzej, sorry for the late reply. I am distracted by several 
> internal projects and other things in my life recently (just came back from 
> vacation). My boss has not decided to let me continue working on LLVM Flang 
> this year, yet.

No worries, thank you for letting us know Peixin!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134821

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

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

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

I've just reverted this patch - for context please see 
https://reviews.llvm.org/D149429.

@sunshaoce This was committed 
 without 
acknowledging @ekieri 's contribution, so didn't follow the official 
guidelines: Attribution of Changes 
. IMHO, we 
should keep Emil as the author of this change (he has done the lion share of 
this highly non-trivial work) and add you as a co-author 
.
 Unless @ekieri has some other preference :) (I am happy as long as we follow 
the guidelines).

Either way, thank you **both** for contributing! Now, lets try to figure out 
what caused the issue with the bots.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134821

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

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

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski accepted this revision.
awarzynski added a comment.

In D134821#4301701 , @sunshaoce wrote:

> Any further suggestions?

I am happy for you to land this, thanks! LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134821

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

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

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for all the great effort, @ekieri !

@sunshaoce , mostly makes sense, just a few small suggestions. @peixin , wdyt?




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

WDYT>



Comment at: flang/test/Driver/link-c-main.c:3
+Test that an object file with a c main function can be linked to an executable
+by flang.
+





Comment at: flang/test/Driver/link-c-main.c:5
+
+For now, this test only covers the Gnu toolchain on linux.
+





Comment at: flang/test/Driver/linker-flags.f90:15
 
+! Check linker invocation to generate shared object (only GNU toolchain for 
now)
+! RUN: %flang -### -flang-experimental-exec -shared -target x86_64-linux-gnu 
%S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,GNU-SHARD 
--implicit-check-not _QQmain

Please, could you clarify that in this case `_QQmain` should not be flagged as 
undefined?



Comment at: flang/test/Driver/linker-flags.f90:61
+! Linker invocation to generate a shared object
+! GNU-SHARD-LABEL:  "{{.*}}ld"
+! GNU-SHARD-SAME: "[[object_file]]"




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134821

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

[PATCH] D148038: [Flang][OpenMP][Driver][MLIR] Port fopenmp-host-ir-file-path flag and add MLIR module attribute to proliferate to OpenMP IR lowering

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D148038#4292549 , @agozillon wrote:

> In D148038#4292262 , @awarzynski 
> wrote:
>
>> LGTM, thanks for addressing my comments!
>
> Thank you for your time and the great review comments as always!

Thanks you 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148038

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

[PATCH] D148038: [Flang][OpenMP][Driver][MLIR] Port fopenmp-host-ir-file-path flag and add MLIR module attribute to proliferate to OpenMP IR lowering

[Andrzej Warzynski via Phabricator via cfe-commits]

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

LGTM, thanks for addressing my comments!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148038

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

[PATCH] D148038: [Flang][OpenMP][Driver][MLIR] Port fopenmp-host-ir-file-path flag and add MLIR module attribute to proliferate to OpenMP IR lowering

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:730
+res.getLangOpts().OMPHostIRFile = arg->getValue();
+if (!llvm::sys::fs::exists(res.getLangOpts().OMPHostIRFile))
+  diags.Report(clang::diag::err_drv_omp_host_ir_file_not_found)

agozillon wrote:
> awarzynski wrote:
> > I think that this is expecting a bit too much from `CompilerInvocation`. 
> > Instead, whatever is going to use this file should complain if the file 
> > does not exist (`CompilerInvocation` is merely a messenger here, and file 
> > I/O can be quite expensive, hence my reservations).
> > 
> > Is it possible to create a test for "invalid file path" wherever this 
> > becomes relevant? 
> I believe the follow up patch I have here which uses the file, emits an error 
> if it can't find it: https://reviews.llvm.org/D148370 
> However, in this case it's an assert rather than more useful Diagnostics 
> unfortunately. 
> 
> Although, this segment of code does currently just mimic what Clang does in 
> it's own 
> `CompilerInvocation`: 
> https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/CompilerInvocation.cpp#L3883
>  
> 
> So it depends if we wish to try to maintain the status quo for the shared 
> argument across the compiler frontends! 
> 
> So whichever you prefer, I am happy to remove the check at this level and let 
> the lowering handle the problem if it arises :-) but I do like sharing the 
> commonality with Clang where possible.
Consistency with Clang is a good idea   Though I would appreciate a test that 
demonstrates that this diagnostic is indeed issues when a non-existing files is 
specified :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148038

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

[PATCH] D148038: [Flang][OpenMP][Driver][MLIR] Port fopenmp-host-ir-file-path flag and add MLIR module attribute to proliferate to OpenMP IR lowering

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: clang/include/clang/Driver/Options.td:6558
   HelpText<"Path to the IR file produced by the frontend for the host.">,
-  Flags<[CC1Option, NoDriverOption]>;
+  Flags<[CC1Option, FC1Option, NoDriverOption]>;
 

With both options using identical `Flags`, could you use `let Flags = `?



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:144
+  // adds other secondary input (e.g. device bitcode files for embedding to the
+  // -fembed-offload-object argument or the host ir file for proccessing
+  // during device compilation to the fopenmp-host-ir-file-path argument via





Comment at: clang/lib/Driver/ToolChains/Flang.cpp:146
+  // during device compilation to the fopenmp-host-ir-file-path argument via
+  // OpenMPDeviceInput) This is condensed logic from the Clang driver
+  // for pushing on further input arguments needed for offloading

[nit] You could refer to a specific method in Clang (I did that in the past in 
a few places)



Comment at: flang/include/flang/Frontend/LangOptions.h:18
 
+#include 
 namespace Fortran::frontend {

[nit] Empty line between



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:730
+res.getLangOpts().OMPHostIRFile = arg->getValue();
+if (!llvm::sys::fs::exists(res.getLangOpts().OMPHostIRFile))
+  diags.Report(clang::diag::err_drv_omp_host_ir_file_not_found)

I think that this is expecting a bit too much from `CompilerInvocation`. 
Instead, whatever is going to use this file should complain if the file does 
not exist (`CompilerInvocation` is merely a messenger here, and file I/O can be 
quite expensive, hence my reservations).

Is it possible to create a test for "invalid file path" wherever this becomes 
relevant? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148038

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

[PATCH] D141307: Add -f[no-]loop-versioning option

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski accepted this revision.
awarzynski added a comment.

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141307

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

[PATCH] D147941: [Flang][Driver][OpenMP] Enable options for selecting offloading phases in flang

[Andrzej Warzynski via Phabricator via cfe-commits]

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

LGTM, thanks for addressing my comments. I'd wait a day before landing this - 
just in case other reviewers have comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147941

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

[PATCH] D147941: [Flang][Driver][OpenMP] Enable flags for filtering of offloading passes in flang

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D147941#4255461 , @skatrak wrote:

> In D147941#4255458 , @awarzynski 
> wrote:
>
>> Could you add tests that demonstrate what these options actually do?
>
> Thank you for the quick review! These options just modify which `flang-new 
> -fc1` invocations are produced by the driver when compiling for device 
> offloading. I have added tests that check that only the expected invocations 
> are present, but if these tests are not what you'd expect I'd gladly make 
> some more if you can explain a bit further what you had in mind.

Cheers!

> These can be used to modify the behavior of the driver to select which 
> compilation passes are triggered during OpenMP offloading.

In the context of LLVM, I would normally associate "pass" with something else. 
I'm not that familiar with offloading, so perhaps that's the language that 
people use? Personally, in the context of the driver I'd normally use the term 
"phase" rather than "pass".

This patch makes sense, though I'd like the following to be addressed before 
landing this:

1. Where's the logic that implements what `--offload-host-device`? It looks 
like this is already implemented elsewhere and this patch simply "unlocks" 
(rather than "implements") this option for Flang.
2. The name of "omp-frontend-forwarding.f90" is very misleading. "Forwarding" 
would mean `flang-new --offload-host-only %s` --> `flang-new -fc1 
--offload-host-only %s`, but that's not what's happening here, is it?

For 1. you could just update the summary, and for 2. I suggest renaming 
"omp-frontend-forwarding.f90" as e.g. "omp-compiler-flag-expansion.f90". WDYT?




Comment at: clang/include/clang/Driver/Options.td:2738
   HelpText<"Don't Use the new driver for offloading compilation.">;
-def offload_device_only : Flag<["--"], "offload-device-only">,
+def offload_device_only : Flag<["--"], "offload-device-only">, 
Flags<[CoreOption, FlangOption]>,
   HelpText<"Only compile for the offloading device.">;

Why is `CoreOption` needed here? Wouldn't `FlangOption` be sufficient?



Comment at: flang/test/Driver/omp-frontend-forwarding.f90:14
+! RUN: --target=aarch64-unknown-linux-gnu \
+! RUN:   | FileCheck %s --check-prefix=CHECK-OFFLOAD-HOST-DEVICE
+

[nit] You can reduce noise by using `OFFLOAD-HOST-DEVICE` instead of 
`CHECK-OFFLOAD-HOST-DEVICE`. I think that most people understand that these are 
`CHECK` prefixes anyway.



Comment at: flang/test/Driver/omp-frontend-forwarding.f90:19
+! RUN: --target=aarch64-unknown-linux-gnu \
+! RUN:   | FileCheck %s --check-prefix=CHECK-OFFLOAD-HOST-DEVICE
+

[nit] No harm in being a bit more verbose to communicate the intent a bit 
clearer :)  



Comment at: flang/test/Driver/omp-frontend-forwarding.f90:30
+
+! CHECK-OFFLOAD-HOST: "{{[^"]*}}flang-new" "-fc1" "-triple" 
"aarch64-unknown-linux-gnu"
+! CHECK-OFFLOAD-HOST-NOT: "-triple" "amdgcn-amd-amdhsa"

Shouldn't there be 2 driver invocation for the host, as in the the 
`CHECK-OFFLOAD-HOST-DEVICE` case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147941

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

[PATCH] D147941: [Flang][Driver][OpenMP] Enable flags for filtering of offloading passes in flang

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Could you add tests that demonstrate what these options actually do?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147941

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

[PATCH] D141307: Add -f[no-]loop-versioning option

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Mats, thanks for working on this! Just a few minor suggestions from me.




Comment at: clang/lib/Driver/ToolChains/Flang.cpp:55
 
+static bool shouldLoopVersion(const ArgList ) {
+  if (Arg *A = Args.getLastArg(options::OPT_Ofast, options::OPT_O,

Could you add a short Docstring, pls? In particular, is the logic implemented 
by this method consistent with GFortran? Are there any external docs that could 
be referred to here?



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:56
+static bool shouldLoopVersion(const ArgList ) {
+  if (Arg *A = Args.getLastArg(options::OPT_Ofast, options::OPT_O,
+   options::OPT_O4, options::OPT_floop_versioning,

Please rewrite this to use [[ 
https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
 | early exit ]], e.g.:
```
  Arg *LoopVersionOption = Args.getLastArg(options::OPT_Ofast, options::OPT_O,
   options::OPT_O4, options::OPT_floop_versioning,
   options::OPT_fno_loop_versioning)
  if !(LoopVersionOption)
return false;

  // The remaining logic HERE
```



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:109-110
   addDebugInfoKind(CmdArgs, DebugInfoKind);
+  if (shouldLoopVersion(Args))
+CmdArgs.push_back("-fversion-loops-for-stride");
 }

Would you classify this as a code-gen option? Alongside `-fstack-arrays` and 
`-flang-experimental-hlfir`? It sound like we could introduce another hook 
here, `adddCodeGenOptions`?



Comment at: flang/test/Driver/version-loops.f90:2
+! Test that flang-new forwards the -f{no-,}version-loops-for-stride 
+! options corredly to flang-new -fc1 for different variants of optimisation
+! and explicit flags.





Comment at: flang/test/Driver/version-loops.f90:5
+
+! RUN: %flang -fsyntax-only -### %s -o %t 2>&1 \
+! RUN:   -O3 \

[nit] If you are using `-###`, then you can just skip `-fsyntax-only` (it 
doesn't really matter what "action" is requested from the frontend driver).



Comment at: flang/test/Driver/version-loops.f90:33-34
+  
+! CHECK: "-fversion-loops-for-stride"
+! CHECK: "-O3"
+

Similar suggestion for other `CHECK` lines - this will make the test a bit more 
explicit about the expected output.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141307

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

[PATCH] D145264: [OpenMP][MLIR][Flang][Driver][bbc] Lower and apply Module FlagsAttr

[Andrzej Warzynski via Phabricator via cfe-commits]

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

@agozillon Thanks for the updates, LGTM! Please wait for somebody to review the 
OpenMP changes before merging.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145264

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

[PATCH] D145264: [OpenMP][MLIR][Flang][Driver][bbc] Lower and apply Module FlagsAttr

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

The driver plumbing looks good to me. I will defer reviewing the OpenMP changes 
to experts. Thanks for working on this!




Comment at: clang/include/clang/Driver/Options.td:2692
 def fno_openmp_assume_teams_oversubscription : Flag<["-"], 
"fno-openmp-assume-teams-oversubscription">,
-  Group, Flags<[CC1Option, NoArgumentUnused, HelpHidden]>;
+  Group, Flags<[CC1Option, FC1Option, NoArgumentUnused, HelpHidden]>;
 def fno_openmp_assume_threads_oversubscription : Flag<["-"], 
"fno-openmp-assume-threads-oversubscription">,

Many of these options have identical flags. While not really needed for this 
change, it would still be nice to re-organise them a bit. This file could 
really benefit from some love :) Here's an example of what I have in mind: 
https://github.com/llvm/llvm-project/blob/cf60d3f1a688671c8eb7859bf0572c403c3c0cca/clang/include/clang/Driver/Options.td#L6575-L6600



Comment at: flang/tools/bbc/CMakeLists.txt:29
 FortranLower
+flangFrontendTool
 )

This a frontend driver library and so far `bbc` and `flang-new -fc1` have been 
entirely separate. Could this dependency be avoided?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145264

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

[PATCH] D146814: [Flang] Add debug flag to enable current debug information pass

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.h:111
+llvm::codegenoptions::DebugInfoKind
+DebugLevelToInfoKind(const llvm::opt::Arg );
+

SBallantyne wrote:
> awarzynski wrote:
> > awarzynski wrote:
> > > CamelCase or camelCase? ;-)
> > Sorry, I meant that this function uses wrong casing. It ought to be 
> > `debugLevelToInfoKind` and `addDebugInfoKind`: 
> > https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
> I thought that the general rule was that the style guide should be followed 
> apart from when there is already code in a particular style in a file, 
> however i have realised that while most functions in this file are 
> `CamelCase`, there are some that are `camelCase`, so i am unsure of what is 
> the right way.
Yes, it's very confusing :(

You are right about the official guidelines (at least that's how I understand 
them). In this case the style used in this file is inconsistent so you can't 
follow that (i.e. because there are multiple styles used here). In situations 
like this I would follow the official style guideline instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146814

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

[PATCH] D145579: [Clang][Flang][AMDGPU] Add support for AMDGPU to Flang driver

[Andrzej Warzynski via Phabricator via cfe-commits]

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

Thanks for implementing this, LGTM!


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

https://reviews.llvm.org/D145579

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

[PATCH] D146814: [Flang] Add debug flag to enable current debug information pass

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for the explanation! Still looking good apart from the function names ;)




Comment at: clang/lib/Driver/ToolChains/CommonArgs.h:111
+llvm::codegenoptions::DebugInfoKind
+DebugLevelToInfoKind(const llvm::opt::Arg );
+

awarzynski wrote:
> CamelCase or camelCase? ;-)
Sorry, I meant that this function uses wrong casing. It ought to be 
`debugLevelToInfoKind` and `addDebugInfoKind`: 
https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146814

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

[PATCH] D146814: [Flang] Add debug flag to enable current debug information pass

[Andrzej Warzynski via Phabricator via cfe-commits]

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

LGTM, thanks for working on this! (please fix the name in CommonArgs.h before 
landing this)

It would be great to see a test that checks for debug info in the generated 
FIR/MLIR/LLVM-IR/ASM file. But that could happen in a separate patch.




Comment at: clang/lib/Driver/ToolChains/CommonArgs.h:111
+llvm::codegenoptions::DebugInfoKind
+DebugLevelToInfoKind(const llvm::opt::Arg );
+

CamelCase or camelCase? ;-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146814

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

[PATCH] D146814: [Flang] Add debug flag to enable current debug information pass

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for the updates! A few more pointers, but nothing major.

Btw, are there any tests that check for debug info in the compiled files? For 
example:

  ! RUN: flang -g1 -S %s | FileCheck -check-prefixes=DEBUG-INFO-PRESENT
  ! RUN: flang -g0 -S %s | FileCheck -check-prefixes=DEBUG-INFO-MISSING
  
  end program




Comment at: clang/lib/Driver/ToolChains/Flang.cpp:32
+static void
+addFortranDebugOptions(ArgStringList ,
+   llvm::codegenoptions::DebugInfoKind DebugInfoKind) {

There isn't anything Fortran specific here, is there? And this method basically 
implements 
https://github.com/llvm/llvm-project/blob/cf60d3f1a688671c8eb7859bf0572c403c3c0cca/clang/lib/Driver/ToolChains/Clang.cpp#L975-L1000,
 right? Why not extract it into e.g. `renderDebugEnablingArgs` and move to 
CommonArgs.cpp?



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:868
   parsePreprocessorArgs(res.getPreprocessorOpts(), args);
-  parseCodeGenArgs(res.getCodeGenOpts(), args, diags);
+  success &= parseCodeGenArgs(res.getCodeGenOpts(), args, diags);
   success &= parseSemaArgs(res, args, diags);

WDYT? I think that it makes sense to keep these separate.



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:144
+val != llvm::codegenoptions::NoDebugInfo) {
+  // TODO: This is not a great warning message, could be improved
+  const auto debugErr = diags.getCustomDiagID(

SBallantyne wrote:
> awarzynski wrote:
> > Please improve it :)
> > 
> > In particular, you are testing for `DebugLineTablesOnly` and `NoDebugInfo`, 
> > yet the diagnostic refers to `-g1`.
> I previously had it emit these debug names, but i think its more confusing 
> for the user as they will be passing `-g2 / -g3` in order to get this error, 
> and the internal name is not as helpful. The TODO was from a previous patch 
> and i just forgot to remove it, i am happy with the current state of warning. 
Thanks for the explanation! My main reservation is that it's not obvious how 
`-g2` and `-g3` map onto `llvm::codegenoptions`. This warnings refers to `-g1`, 
but there's no mention of `-g1` in this function. 

TBH, I would replace "Debug options greater than -g1 not yet implemented" with 
something a bit more generic and future-proof, e.g. `"Unsupported debug option: 
%0" << arg.getValue()`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146814

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

[PATCH] D145579: [Clang][Flang][AMDGPU] Add support for AMDGPU to Flang driver

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for the updates, mostly looks good. Just a couple of extra questions 
about the test coverage.




Comment at: flang/lib/Frontend/FrontendActions.cpp:139-142
+  // Clang does not append all target features to the clang -cc1 invocation.
+  // Some AMDGPU features are passed implicitly by the Clang frontend.
+  // That's why we need to extract implicit AMDGPU target features and add
+  // them to the target features specified by the user

domada wrote:
> awarzynski wrote:
> > domada wrote:
> > > awarzynski wrote:
> > > > [nit] IMHO this is documenting an implementation detail that would be 
> > > > more relevant inside `getExplicitAndImplicitAMDGPUTargetFeatures`.
> > > > 
> > > > More importantly, you are suggesting that the driver is doing whatever 
> > > > it is doing "because that's what Clang does". Consistency with Clang is 
> > > > important (you could call it out in the commit message) :) However, it 
> > > > would be even nicer to understand the actual rationale behind these 
> > > > implicit features. Any idea? Perhaps there are some clues in git 
> > > > history?
> > > > 
> > > > Also, perhaps a TODO to share this code with Clang? (to make it even 
> > > > clearer that the frontend driver aims for full consistency with Clang 
> > > > here).
> > > I think that the main difference between Clang and Flang is the lack of 
> > > `TargetInfo` class.
> > > 
> > > [[ https://clang.llvm.org/doxygen/classclang_1_1TargetInfo.html | 
> > > TargetInfo classes ]] are Clang specific and they are responsible for 
> > > parsing/adding default target features. Every target performs 
> > > initialization in different way (compare for example [[ 
> > > https://clang.llvm.org/doxygen/Basic_2Targets_2AArch64_8cpp_source.html#l00960
> > >  | AArch64 ]] vs [[ 
> > > https://clang.llvm.org/doxygen/Basic_2Targets_2AMDGPU_8cpp_source.html#l00179
> > >  | AMDGPU ]] target initialization.
> > > 
> > > I don't want to make TargetInfo class Clang indendent (see discussion: 
> > > https://discourse.llvm.org/t/rfc-targetinfo-library/64342 ). I also don't 
> > > want to reimplement the whole TargetInfo class in Flang, because Flang 
> > > already uses LLVM TargetMachine class (see: 
> > > https://llvm.org/doxygen/classllvm_1_1TargetMachine.html and 
> > > https://github.com/llvm/llvm-project/blob/main/flang/lib/Frontend/FrontendActions.cpp#L614
> > >  )  which can play similar role as Clang TargetInfo IMO.
> > > 
> > > That's why I decided to implement 
> > > `getExplicitAndImplicitAMDGPUTargetFeatures` function which performs 
> > > initialization of default AMDGPU target features.
> > Thanks for this comprehensive overview! It would be helpful if this 
> > rationale was included in the summary (in the spirit of documenting things 
> > for our future selves).
> > 
> > So, there isn't anything special about AMDGPU here, is there? We will have 
> > to implement similar hooks for other targets at some point too, right? Or 
> > perhaps there's some reason to do this for AMDGPU sooner rather than later?
> > 
> > I'm not against this change, just want to better understand the wider 
> > context.
> Hi,
> I modified the comment above to be more informative.
> 
> IMO, we need to add similar hooks for other targets. For example:
> 
> ```
> clang --target=aarch64 t.c -S -emit-llvm -v 
> // I see in the logs explicit target features:
> // +neon,  +v8a ,  -fmv
> // However, generated t.ll contains 4 target features:
> "target-features"="+fp-armv8,+neon,+v8a,-fmv"
> // It looks like target feature +fp-armv8 is implicit
> ```
Thanks for checking!



Comment at: flang/lib/Frontend/FrontendActions.cpp:147-151
+llvm::SMDiagnostic err;
+err.print(errorMsg.data(), llvm::errs());
+unsigned diagID = ci.getDiagnostics().getCustomDiagID(
+clang::DiagnosticsEngine::Error, "Unsupported feature ID");
+ci.getDiagnostics().Report(diagID);





Comment at: flang/lib/Frontend/FrontendActions.cpp:149
+err.print(errorMsg.data(), llvm::errs());
+unsigned diagID = ci.getDiagnostics().getCustomDiagID(
+clang::DiagnosticsEngine::Error, "Unsupported feature ID");

Are you able to add a test that will trigger this?



Comment at: flang/test/Driver/target-cpu-features.f90:59
+! CHECK-AMDGPU-R600: "-fc1" "-triple" "r600-unknown-unknown"
+! CHECK-AMDGPU-R600-SAME: "-target-cpu" "cayman"

Hm, there aren't any "implicit" target features here.


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

https://reviews.llvm.org/D145579

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

[PATCH] D142347: [NFC][Clang] Move DebugOptions to llvm/Frontend for reuse in Flang

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski accepted this revision.
awarzynski added a comment.

LGTM, thanks! As @kiranchandramohan points out, this is in line with the 
overall direction that we agreed on a while back.

This change will very likely affect various downstream consumers - it would be 
helpful to advertise this with a PSA on Discourse.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142347

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

[PATCH] D146814: [Flang] Add debug flag to enable current debug information pass

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

What's the overall design goal here? 100% consistency with Clang? Could this be 
documented?




Comment at: clang/include/clang/Driver/ToolChain.h:23
 #include "llvm/ADT/StringRef.h"
-#include "llvm/ADT/Triple.h"
 #include "llvm/Frontend/Debug/Options.h"

Unrelated change?



Comment at: clang/lib/Driver/ToolChains/Clang.h:16
 #include "clang/Driver/Types.h"
-#include "llvm/ADT/Triple.h"
 #include "llvm/Frontend/Debug/Options.h"

Unrelated change?



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:33
+// to the corresponding DebugInfoKind.
+static llvm::codegenoptions::DebugInfoKind DebugLevelToInfoKind(const Arg ) {
+  assert(A.getOption().matches(options::OPT_gN_Group) &&

Looks identical to 
https://github.com/llvm/llvm-project/blob/cf60d3f1a688671c8eb7859bf0572c403c3c0cca/clang/lib/Driver/ToolChains/Clang.cpp#L406-L420.
 Why not move to 
https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/CommonArgs.cpp?



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:127
+unsigned val =
+llvm::StringSwitch(arg->getValue())
+.Case("line-tables-only", 
llvm::codegenoptions::DebugLineTablesOnly)

1. Why `unsigned` instead of [[ 
https://github.com/llvm/llvm-project/blob/cf60d3f1a688671c8eb7859bf0572c403c3c0cca/clang/include/clang/Basic/DebugInfoOptions.h#L20-L55
 | DebugInfoKind ]]
2. Why not use `std::optional`, e.g. 
`llvm::StringSwitch>arg->getValue())`? This way 
you could avoid magic numbers like `~0U`.



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:137
+if (val == ~0U) {
+  diags.Report(clang::diag::err_drv_invalid_value)
+  << arg->getAsString(args) << arg->getValue();

Please test this diagnostic. Also, if this is an error then you should return 
immediately and signal that this method has failed (e.g. `return false;`).



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:144
+val != llvm::codegenoptions::NoDebugInfo) {
+  // TODO: This is not a great warning message, could be improved
+  const auto debugErr = diags.getCustomDiagID(

Please improve it :)

In particular, you are testing for `DebugLineTablesOnly` and `NoDebugInfo`, yet 
the diagnostic refers to `-g1`.



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:145
+  // TODO: This is not a great warning message, could be improved
+  const auto debugErr = diags.getCustomDiagID(
+  clang::DiagnosticsEngine::Warning,

Looks like you are creating a warning rather than an error: `debugErr` --> 
`debugWarn`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146814

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

[PATCH] D145579: [Flang][AMDGPU] Add support for AMDGPU to Flang driver

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/lib/Frontend/FrontendActions.cpp:139-142
+  // Clang does not append all target features to the clang -cc1 invocation.
+  // Some AMDGPU features are passed implicitly by the Clang frontend.
+  // That's why we need to extract implicit AMDGPU target features and add
+  // them to the target features specified by the user

domada wrote:
> awarzynski wrote:
> > [nit] IMHO this is documenting an implementation detail that would be more 
> > relevant inside `getExplicitAndImplicitAMDGPUTargetFeatures`.
> > 
> > More importantly, you are suggesting that the driver is doing whatever it 
> > is doing "because that's what Clang does". Consistency with Clang is 
> > important (you could call it out in the commit message) :) However, it 
> > would be even nicer to understand the actual rationale behind these 
> > implicit features. Any idea? Perhaps there are some clues in git history?
> > 
> > Also, perhaps a TODO to share this code with Clang? (to make it even 
> > clearer that the frontend driver aims for full consistency with Clang here).
> I think that the main difference between Clang and Flang is the lack of 
> `TargetInfo` class.
> 
> [[ https://clang.llvm.org/doxygen/classclang_1_1TargetInfo.html | TargetInfo 
> classes ]] are Clang specific and they are responsible for parsing/adding 
> default target features. Every target performs initialization in different 
> way (compare for example [[ 
> https://clang.llvm.org/doxygen/Basic_2Targets_2AArch64_8cpp_source.html#l00960
>  | AArch64 ]] vs [[ 
> https://clang.llvm.org/doxygen/Basic_2Targets_2AMDGPU_8cpp_source.html#l00179 
> | AMDGPU ]] target initialization.
> 
> I don't want to make TargetInfo class Clang indendent (see discussion: 
> https://discourse.llvm.org/t/rfc-targetinfo-library/64342 ). I also don't 
> want to reimplement the whole TargetInfo class in Flang, because Flang 
> already uses LLVM TargetMachine class (see: 
> https://llvm.org/doxygen/classllvm_1_1TargetMachine.html and 
> https://github.com/llvm/llvm-project/blob/main/flang/lib/Frontend/FrontendActions.cpp#L614
>  )  which can play similar role as Clang TargetInfo IMO.
> 
> That's why I decided to implement 
> `getExplicitAndImplicitAMDGPUTargetFeatures` function which performs 
> initialization of default AMDGPU target features.
Thanks for this comprehensive overview! It would be helpful if this rationale 
was included in the summary (in the spirit of documenting things for our future 
selves).

So, there isn't anything special about AMDGPU here, is there? We will have to 
implement similar hooks for other targets at some point too, right? Or perhaps 
there's some reason to do this for AMDGPU sooner rather than later?

I'm not against this change, just want to better understand the wider context.


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

https://reviews.llvm.org/D145579

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

[PATCH] D146814: [Flang] Add debug flag to enable current debug information pass

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for submitting this! Please add tests for `-g` and all variants of 
`-gline-tables-only`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146814

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

[PATCH] D146075: [flang][driver][openmp] Write MLIR for -save-temps

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski accepted this revision.
awarzynski added a comment.

LGTM!




Comment at: flang/test/Driver/save-temps.f90:14
 ! CHECK-NEXT: "-o" "save-temps.o"
 ! CHECK-NEXT: "-o" "a.out"
 

skatrak wrote:
> awarzynski wrote:
> > skatrak wrote:
> > > awarzynski wrote:
> > > > Why are there no MLIR files here? Same comment for other invocations.
> > > This is because the general way in which -save-temps works is different 
> > > from what's implemented in this patch for MLIR in flang. In the other 
> > > cases, the driver splits the work into several frontend invocations, 
> > > where each step generally produces the input of the next. `-save-temps` 
> > > makes sure these intermediate files are kept where the user specified and 
> > > not deleted.
> > > 
> > > In this patch, instead of modifying the driver to create a frontend 
> > > invocation to produce MLIR (generating the *-fir.mlir file), another one 
> > > to optimize/lower that (generating the *-llvmir.mlir file), and a third 
> > > one to translate lowered MLIR into LLVM IR, we just forward the flag to 
> > > the frontend, which creates extra MLIR files at particular spots of the 
> > > codegen process if the flag is set. Hence, MLIR files don't show in the 
> > > output of `flang-new -###`.
> > So, IIUC, without `-emit-llvm-bc` there should be no intermediate MLIR 
> > files? I would add `CHECK-NOT`.
> Actually, with this approach there are no changes to the output of `flang 
> -###` either way. I was using `-fc1 -emit-llvm-bc` just because that triggers 
> both MLIR temp outputs (the one before any optimizations/lowering, and the 
> one right before LLVM IR generation), but I simplified that a bit by just 
> using `flang -c` instead and avoid confusion.
Ah, I missed that, good point!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146075

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

[PATCH] D146075: [flang][driver][openmp] Write MLIR for -save-temps

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski accepted this revision.
awarzynski added a comment.

This is a very nice patch, thanks for working on this! A few final nits, but 
feel free to ignore. LGTM




Comment at: flang/lib/Frontend/FrontendActions.cpp:103
+  std::error_code ec;
+  llvm::ToolOutputFile out(outDir + outFile, ec, llvm::sys::fs::OF_Text);
+  if (ec)

skatrak wrote:
> awarzynski wrote:
> > Why not just [[ 
> > https://github.com/llvm/llvm-project/blob/3b1951aceb8c58acd3d5c5ba2d042ad3d03c13c4/flang/lib/Frontend/FrontendActions.cpp#L793
> >  | print ]] the MLIR module?
> I'm not sure I completely understand the issue here, by looking at the code 
> you linked to. Maybe it's related to the fact that this function is 
> implemented by creating new files apart from the main output of the current 
> compiler invocation. I developed the idea a bit more in the reply to your 
> comment on "save-temps.f90".
Apologies, I am blind - you were using `print` already!



Comment at: flang/test/Driver/save-temps.f90:58
+!--
+! MLIR generation
+!--

[nit] For MLIR you check the contents of the intermediate files (great!), but 
then for all other temp files we don't (some of them are binary and that's one 
reason). Given that testing for MLIR temp files is so different, I'd be tempted 
to move to a different file. Or at least add a comment here to explain the 
rationale for testing differently.



Comment at: flang/test/Driver/save-temps.f90:14
 ! CHECK-NEXT: "-o" "save-temps.o"
 ! CHECK-NEXT: "-o" "a.out"
 

skatrak wrote:
> awarzynski wrote:
> > Why are there no MLIR files here? Same comment for other invocations.
> This is because the general way in which -save-temps works is different from 
> what's implemented in this patch for MLIR in flang. In the other cases, the 
> driver splits the work into several frontend invocations, where each step 
> generally produces the input of the next. `-save-temps` makes sure these 
> intermediate files are kept where the user specified and not deleted.
> 
> In this patch, instead of modifying the driver to create a frontend 
> invocation to produce MLIR (generating the *-fir.mlir file), another one to 
> optimize/lower that (generating the *-llvmir.mlir file), and a third one to 
> translate lowered MLIR into LLVM IR, we just forward the flag to the 
> frontend, which creates extra MLIR files at particular spots of the codegen 
> process if the flag is set. Hence, MLIR files don't show in the output of 
> `flang-new -###`.
So, IIUC, without `-emit-llvm-bc` there should be no intermediate MLIR files? I 
would add `CHECK-NOT`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146075

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

[PATCH] D145579: [Flang][AMDGPU] Add support for AMDGPU to Flang driver

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

A few more comments, but mostly nits. Btw, is this patch sufficient to generate 
code for AMDGPU? Or, put differently, what's the level of support atm?




Comment at: clang/lib/Driver/ToolChains/Flang.cpp:107
 break;
+  case llvm::Triple::r600:
+  case llvm::Triple::amdgcn:

Should there be a test for this triple as well?



Comment at: flang/lib/Frontend/FrontendActions.cpp:134-137
+  if (!triple.isAMDGPU()) {
+return llvm::join(targetOpts.featuresAsWritten.begin(),
+  targetOpts.featuresAsWritten.end(), ",");
+  }

[nit] I would probably flip this as:
```
if (triple.isAMDGPU()) {
  // Clang does not append all target features to the clang -cc1 invocation.
  // Some AMDGPU features are passed implicitly by the Clang frontend.
  // That's why we need to extract implicit AMDGPU target features and add
  // them to the target features specified by the user
  return getExplicitAndImplicitAMDGPUTargetFeatures(ci, targetOpts, triple);
  }
``` 

I know that I suggested the opposite in my previous comment, but you have 
simplified this since :) In any case, feel free to ignore.



Comment at: flang/lib/Frontend/FrontendActions.cpp:139-142
+  // Clang does not append all target features to the clang -cc1 invocation.
+  // Some AMDGPU features are passed implicitly by the Clang frontend.
+  // That's why we need to extract implicit AMDGPU target features and add
+  // them to the target features specified by the user

[nit] IMHO this is documenting an implementation detail that would be more 
relevant inside `getExplicitAndImplicitAMDGPUTargetFeatures`.

More importantly, you are suggesting that the driver is doing whatever it is 
doing "because that's what Clang does". Consistency with Clang is important 
(you could call it out in the commit message) :) However, it would be even 
nicer to understand the actual rationale behind these implicit features. Any 
idea? Perhaps there are some clues in git history?

Also, perhaps a TODO to share this code with Clang? (to make it even clearer 
that the frontend driver aims for full consistency with Clang here).


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

https://reviews.llvm.org/D145579

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

[PATCH] D146278: [flang] add -flang-experimental-hlfir flag to flang-new

[Andrzej Warzynski via Phabricator via cfe-commits]

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

LGTM, thanks!




Comment at: flang/test/HLFIR/flang-experimental-hlfir-flag.f90:19
+! CHECK-NEXT:  }
+! NO-HLFIR-NOT: hlfir.

[ultra nit] I would separate with an empty line for better readability. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146278

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

[PATCH] D145579: [Flang][AMDGPU][OpenMP] Save target features in OpenMP MLIR dialect

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Really nice to see some shared code being elevated out of Clang into LLVM, 
thanks!

I've only reviewed on the Flang driver changes. I will let the OpenMP experts 
to review the remaining bits. All in all looks good, I've only made some small 
suggestions.

Thanks for working on this!




Comment at: flang/lib/Frontend/FrontendActions.cpp:93
 
+std::string CodeGenAction::getAllTargetFeatures() {
+  std::string allFeaturesStr;

This method could be simplified by following 
https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code.
 For example:

```
std::string CodeGenAction::getAllTargetFeatures()  {
  if (!triple.isAMDGPU()) {
allFeaturesStr = llvm::join(targetOpts.featuresAsWritten.begin(),
targetOpts.featuresAsWritten.end(), ",");
return allFeaturesStr;
  }

  // The logic for AMDGPU
  // Perhaps add a dedicated hook: getExplicitAndImplicitAMDGPUTargetFeatures()
} 
```

Btw, this method does different things depending on the triple. Perhaps rename 
it as something more generic, e.g. `getTargetFeatures`? I think that the 
current name, `getAllTargetFeatures`, is a bit misleading (i.e. what does "all" 
mean?).

Also:
* make it `static`
* document


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

https://reviews.llvm.org/D145579

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

[PATCH] D146278: [flang] add -flang-experimental-hlfir flag to flang-new

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/test/HLFIR/flang-experimental-hlfir-flag.f90:2
+! Test -flang-experimental-hlfir flag
+! RUN: %flang_fc1 -flang-experimental-hlfir -emit-fir -o - %s | FileCheck %s
+

Could you also add a `RUN` line like this:
```
! RUN: %flang_fc1 -emit-fir -o - %s | FileCheck %s --prefix="NO-HLFIR"

! CHECK-NOT: hlfir
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146278

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

[PATCH] D146075: [flang][driver][openmp] Write MLIR for -save-temps

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for the updates!

I advice against using UPPER case in filenames. Please bear in mind that on 
Windows and MacOS filenames are case insensitive. It's just less hassle to 
stick to lower case.




Comment at: flang/lib/Frontend/FrontendActions.cpp:79
+ llvm::StringRef outputTag) {
+  if (!ci.getCodeGenOpts().SaveTempsDir.has_value())
+return true;

This is confusing - `-save-temps` should work even when no directory is 
specified.



Comment at: flang/lib/Frontend/FrontendActions.cpp:82-83
+
+  const std::string  = ci.getFrontendOpts().outputFile;
+  const std::string  = ci.getCodeGenOpts().SaveTempsDir.value();
+  std::string outDir =

Please use LLVM's containers instead (e.g. `SmallString` or `StringRef`)



Comment at: flang/lib/Frontend/FrontendActions.cpp:86-87
+  llvm::StringSwitch(dir)
+  .Case("cwd", "")
+  .Case("obj", llvm::sys::path::parent_path(compilerOutFile).str())
+  .Default(dir);

Could you test both variants?



Comment at: flang/lib/Frontend/FrontendActions.cpp:96-100
+  std::string outFile = input.substr(0, input.find_last_of('.'))
+.str()
+.append("-")
+.append(outputTag.str())
+.append(".mlir");

Please use hooks from [[ 
https://github.com/llvm/llvm-project/blob/3b1951aceb8c58acd3d5c5ba2d042ad3d03c13c4/llvm/include/llvm/Support/Path.h#L213-L229
 | Path.h ]] for path manipulation.



Comment at: flang/lib/Frontend/FrontendActions.cpp:103
+  std::error_code ec;
+  llvm::ToolOutputFile out(outDir + outFile, ec, llvm::sys::fs::OF_Text);
+  if (ec)

Why not just [[ 
https://github.com/llvm/llvm-project/blob/3b1951aceb8c58acd3d5c5ba2d042ad3d03c13c4/flang/lib/Frontend/FrontendActions.cpp#L793
 | print ]] the MLIR module?



Comment at: flang/lib/Frontend/FrontendActions.cpp:624
 
+  // LLVMIR.mlir: This is 2nd -save-temps file created for mlir
+  if (!saveMLIRTempFile(ci.getInvocation(), *mlirModule, getCurrentFile(),

IMHO, the fact that this is the 2nd file is not that relevant (what people 
start printing more files and this becomes the 3rd file?). But I would 
appreciate a comment explaining that this is the "LLVM IR MLIR file" (i.e. the 
MLIR file just before lowering to LLVM IR). Similar comment for FIR.mlir.



Comment at: flang/lib/Frontend/FrontendActions.cpp:628
+unsigned diagID = ci.getDiagnostics().getCustomDiagID(
+clang::DiagnosticsEngine::Error, "Saving MLIR temp file failed");
+ci.getDiagnostics().Report(diagID);

Could you add a test for this diagnostic? You could try by specifying an 
invalid output dir.



Comment at: flang/test/Driver/save-temps.f90:14
 ! CHECK-NEXT: "-o" "save-temps.o"
 ! CHECK-NEXT: "-o" "a.out"
 

Why are there no MLIR files here? Same comment for other invocations.



Comment at: flang/test/Driver/save-temps.f90:61
+! RUN: rm -rf %t && mkdir -p %t
+! RUN: %flang_fc1 -emit-llvm-bc -save-temps=obj -o %t/out.bc %s 2>&1
+! RUN: FileCheck %s -input-file=%t/save-temps-FIR.mlir -check-prefix=MLIR-FIR

What happens in this case: `-save-temps`? (no dir is specified)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146075

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

[PATCH] D146278: [flang] add -flang-experimental-hlfir flag to flang-new

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/test/Driver/mlir-pass-pipeline.f90:15-18
+! O2-NEXT: Canonicalizer
+! ALL-NEXT: LowerHLFIRIntrinsics
+! ALL-NEXT: BufferizeHLFIR
+! ALL-NEXT: ConvertHLFIRtoFIR

tblah wrote:
> awarzynski wrote:
> > It looks like these passes are run unconditionally - what's the 
> > `-flang-experimental-hlfir` flag for then?
> Yes. @vzakhari requested the passes should run unconditionally.
> 
> `-flang-experimental-hlfir` controls whether the HLFIR lowering is used. 
Ok, so so far you have updated the default pass pipelines and that's what you 
are testing here. However, this patch is about `-flang-experimental-hlfir` - 
can you add a test for this flag?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146278

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

[PATCH] D146278: [flang] add -flang-experimental-hlfir flag to flang-new

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/test/Driver/mlir-pass-pipeline.f90:15-18
+! O2-NEXT: Canonicalizer
+! ALL-NEXT: LowerHLFIRIntrinsics
+! ALL-NEXT: BufferizeHLFIR
+! ALL-NEXT: ConvertHLFIRtoFIR

It looks like these passes are run unconditionally - what's the 
`-flang-experimental-hlfir` flag for then?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146278

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

[PATCH] D146278: [flang] add -flang-experimental-hlfir flag to flang-new

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Could you add some tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146278

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

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/test/Driver/omp-frontend-forwarding.f90:1
+! REQUIRES: amdgpu-registered-target
+

jhuber6 wrote:
> agozillon wrote:
> > awarzynski wrote:
> > > Given that you use `-###`, I think that this can be skipped (please 
> > > double check).
> > It does appear that it can be, at the very least I can swap in an NVIIDIA 
> > arch when I haven't configured the project to target it and it has no 
> > issues! Thank you. 
> I'm not completely familiar with Flangs status on this, do we have tests in 
> tree that perform the entire build and check `-ccc-print-bindings/phases` 
> like we do in Clang?
> I'm not completely familiar with Flangs status on this

I don't recall any other efforts to support offloading in Flang's compiler 
driver - very nice to see this being worked on!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145815

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

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Andrzej Warzynski via Phabricator via cfe-commits]

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

LGTM, thanks for working on this! Would be great for somebody with a bit more 
experience with offloading to OK this as well :) @tschuett or perhaps @jhuber6 ?




Comment at: clang/lib/Driver/ToolChains/Flang.cpp:128
+  if (IsHostOffloadingAction) {
+for (size_t i = 1; i < Inputs.size(); ++i) {
+  if (Inputs[i].getType() != types::TY_Nothing)

jhuber6 wrote:
> agozillon wrote:
> > jhuber6 wrote:
> > > agozillon wrote:
> > > > awarzynski wrote:
> > > > > agozillon wrote:
> > > > > > awarzynski wrote:
> > > > > > > What's the magic "1"? And given that the input count matters here 
> > > > > > > - is there a test with multiple inputs?
> > > > > > It aims to mimic the behavior of Clang: 
> > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L4561
> > > > > >  where the main input is skipped (the input currently being 
> > > > > > compiled or embedded into etc.), when adding to 
> > > > > > //-fembed-offload-object//. 
> > > > > > 
> > > > > > It does look different to Clang's as Clang has more cases and the 
> > > > > > logic is spread across the constructJob invocation, but the first 
> > > > > > if case is what the if statement inside of the loop and setting the 
> > > > > > loop index variable to 1 do. The HostOffloadingInputs array is what 
> > > > > > is being generated here, except we're skipping and directly 
> > > > > > applying it as arguments.
> > > > > > 
> > > > > > I tried to condense it a little in this case! Perhaps it loses 
> > > > > > readability though, I had hoped the comment might have kept it clear
> > > > > Thanks for the link - that code in Clang doesn't really clarify what 
> > > > > makes `Inputs[0]` special 樂 . 
> > > > > 
> > > > > Let me rephrase my question - what's so special about the first 
> > > > > input? (referred to in Clang as "main input") Is that something 
> > > > > specific to OpenMP? For example, in this case:
> > > > > ```
> > > > > flang-new  -fopenmp  file.f90
> > > > > ```
> > > > > I assume that `inputs[0]` is "file.f90", so nothing will happen?
> > > > > 
> > > > > > I tried to condense it a little in this case! Perhaps it loses 
> > > > > > readability though, I had hoped the comment might have kept it clear
> > > > > 
> > > > > Nah, I think that your implementation is fine. It's my ignorance with 
> > > > > respect to OpenMP that's the problem here ;-)
> > > > It's not specific to OpenMP I believe, as far as I am aware Clang's 
> > > > supported offload models (SYCL and CUDA as well as OpenMP) use it! In 
> > > > Flang's case we only really care about OpenMP as I believe it's the 
> > > > only offload programming model supported.
> > > > 
> > > > In the case of the command: 
> > > > 
> > > > ```
> > > > flang-new -fopenmp file.f90
> > > > ``` 
> > > > The code should never be executed as no part of the command will enable 
> > > > an offloading action (for device or host)! But yes inputs[0] would 
> > > > indeed refer to file.f90.
> > > > 
> > > > However, this code becomes relevant when you utilise an option that 
> > > > enables the clangDriver to perform some form of offloading action. For 
> > > > example a command like: 
> > > > 
> > > > ```
> > > > flang-new -fopenmp --offload-arch=gfx90a file.f90 
> > > > ```
> > > > Will trigger two phase compilation, one for the host device (your 
> > > > resident CPU in this command) and one for the device (gfx90a in this 
> > > > command), the regular host pass will invoke like your provided command 
> > > > and the device pass will then invoke with -fopenmp-is-device in 
> > > > addition alongside the device triple. This generates two bitcode files 
> > > > from the one file, one containing the host code from the file, the 
> > > > other the device code (extracted from OpenMP target regions or declare 
> > > > target etc.). 
> > > > 
> > > > However, now we have two files, with both parts of our program, we need 
> > > > to conjoin them together, the clangDriver generates an embeddable 
> > > > fat-binary/binary using the clang-offload-packager and then invokes 
> > > > flang-new again, and this is where the above code becomes relevant, as 
> > > > this binary (or multiple binaries, if we target multiple devices in the 
> > > > same program) is what is passed to -fembed-offload-object! And 
> > > > inputs[0] in this case would refer to the output from the original host 
> > > > pass, which is what we want to embed the device binary into, so we wish 
> > > > to skip this original host output and only pass the subsequent inputs 
> > > > (which should be device binaries when the clangDriver initiates a host 
> > > > offloading action) we want to embed as -fembed-offload-object 
> > > > arguments. 
> > > > 
> > > > The offloading driver is quite complex and my knowledge of it is not 
> > > > perfect as I am not our resident 

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:128
+  if (IsHostOffloadingAction) {
+for (size_t i = 1; i < Inputs.size(); ++i) {
+  if (Inputs[i].getType() != types::TY_Nothing)

agozillon wrote:
> awarzynski wrote:
> > What's the magic "1"? And given that the input count matters here - is 
> > there a test with multiple inputs?
> It aims to mimic the behavior of Clang: 
> https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L4561
>  where the main input is skipped (the input currently being compiled or 
> embedded into etc.), when adding to //-fembed-offload-object//. 
> 
> It does look different to Clang's as Clang has more cases and the logic is 
> spread across the constructJob invocation, but the first if case is what the 
> if statement inside of the loop and setting the loop index variable to 1 do. 
> The HostOffloadingInputs array is what is being generated here, except we're 
> skipping and directly applying it as arguments.
> 
> I tried to condense it a little in this case! Perhaps it loses readability 
> though, I had hoped the comment might have kept it clear
Thanks for the link - that code in Clang doesn't really clarify what makes 
`Inputs[0]` special 樂 . 

Let me rephrase my question - what's so special about the first input? 
(referred to in Clang as "main input") Is that something specific to OpenMP? 
For example, in this case:
```
flang-new  -fopenmp  file.f90
```
I assume that `inputs[0]` is "file.f90", so nothing will happen?

> I tried to condense it a little in this case! Perhaps it loses readability 
> though, I had hoped the comment might have kept it clear

Nah, I think that your implementation is fine. It's my ignorance with respect 
to OpenMP that's the problem here ;-)



Comment at: clang/test/Driver/flang/flang-omp.f90:6
+! Test regular -fopenmp with no offload
+! RUN: %clang --driver-mode=flang -### -fopenmp %s 2>&1 | FileCheck 
--check-prefixes=CHECK-OPENMP %s
+! CHECK-OPENMP: "{{[^"]*}}flang-new" "-fc1" {{.*}} "-fopenmp" {{.*}}.f90"

agozillon wrote:
> awarzynski wrote:
> > Can you remind me why isn't it possible to test this with `flang-new`? 
> I double checked, it seems possible to test these with flang-new directly, 
> the main reason I've tested it like this is as it's based on the other tests 
> in the same directory which use clang! 
> 
> However, I'm more than happy to move these tests to the 
> omp-frontend-forwarding.f90 test, remove them or keep these and add flang-new 
> variations into omp-frontend-forwarding.f90. 
I know that it's a bit confusing - Flang.cpp lives in Clang rather than Flang. 
But that's because `Flang.cpp` is part of `clangDriver` - the driver library. 
That library is shared between Clang and Flang and in principle should be taken 
out of Clang into a dedicated subproject - that's the plan :)

This is effectively a Flang patch and I would prefer this test to be added in 
Flang (with `clang` being replaced with `flang-new`). In general, I wouldn't 
add any test in Clang unless testing something Clang specific. This test, 
however, tests Flang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145815

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

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Andrzej Warzynski via Phabricator via cfe-commits]

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



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:128
+  if (IsHostOffloadingAction) {
+for (size_t i = 1; i < Inputs.size(); ++i) {
+  if (Inputs[i].getType() != types::TY_Nothing)

What's the magic "1"? And given that the input count matters here - is there a 
test with multiple inputs?



Comment at: clang/test/Driver/flang/flang-omp.f90:6
+! Test regular -fopenmp with no offload
+! RUN: %clang --driver-mode=flang -### -fopenmp %s 2>&1 | FileCheck 
--check-prefixes=CHECK-OPENMP %s
+! CHECK-OPENMP: "{{[^"]*}}flang-new" "-fc1" {{.*}} "-fopenmp" {{.*}}.f90"

Can you remind me why isn't it possible to test this with `flang-new`? 



Comment at: flang/test/Driver/omp-frontend-forwarding.f90:13
+! CHECK: "{{[^"]*}}flang-new" "-fc1" "-triple" "aarch64-unknown-linux-gnu" 
{{.*}} "-fopenmp" {{.*}}.f90"
+! CHECK-NOT: "{{[^"]*}}flang-new" "-fc1" "-triple" "aarch64-unknown-linux-gnu" 
{{.*}} "-fopenmp" {{.*}} "-fopenmp-is-device" {{.*}}.f90"
+! CHECK: "{{[^"]*}}flang-new" "-fc1" "-triple" "amdgcn-amd-amdhsa" {{.*}} 
"-fopenmp" {{.*}} "-fopenmp-is-device" {{.*}}.f90"

agozillon wrote:
> awarzynski wrote:
> > I feel that you can safely remove this line and replace the following with 
> > `CHECK-NEXT`, no? Just wondering whether there's a way to reduce the noise 
> > in these tests (without sacrificing the rigor).
> I can indeed, thank you very much @awarzynski! I have a knack for 
> overcomplicating these it seems
>  I have a knack for overcomplicating these it seems

You should see one of my patches ;-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145815

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

[PATCH] D145883: [Flang][RISCV] Emit target features for RISC-V

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/test/Driver/code-gen-rv64.f90:1
+! Test -emit-obj (RISC-V 64)
+

mamrami wrote:
> Hi :)
> It seems like the test fails: 
> https://buildkite.com/llvm-project/premerge-checks/builds/141302#0186e55e-199e-401d-ab9f-9f3d47ec87af
> I see it in my non related patch https://reviews.llvm.org/D146132
Sorry about that :(

```
 line 3: llvm-readelf: command not found
```

There are 3 options:
* revert
* add `llvm-readelf` here 
https://github.com/llvm/llvm-project/blob/main/flang/test/CMakeLists.txt#L61
* updated the test to use `llvm-objdump` instead

HTH.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145883

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

[PATCH] D146075: [flang][driver][openmp] Write MLIR for -save-temps

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D146075#4196998 , @tschuett wrote:

> What is the policy on trivial braces in the frontend?

What do you mean by the frontend? Different people have different understanding 
of what "the frontend" is.

I try to follow 
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146075

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

[PATCH] D146075: [flang][driver][openmp] Write MLIR for -save-temps

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Please add tests ;)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146075

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

[PATCH] D145883: [Flang][RISCV] Emit target features for RISC-V

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/test/Driver/target-cpu-features-invalid.f90:13
+! CHECK-INVALID-CPU: 'supercpu' is not a recognized processor for this target 
(ignoring processor)
+! CHECK-INVALID-FEATURE: '+superspeed' is not a recognized feature for this 
target (ignoring feature)

jrtc27 wrote:
> awarzynski wrote:
> > jrtc27 wrote:
> > > Don't these come from the backend? Testing them here doesn't seem right...
> > > Don't these come from the backend?
> > 
> > No. Both options are defined in Clang's Options.td:
> > * [[ 
> > https://github.com/llvm/llvm-project/blob/0aac9a2875bad4f065367e4a6553fad78605f895/clang/include/clang/Driver/Options.td#L5251-L5253
> >  | -target-feaure ]]
> > * [[ 
> > https://github.com/llvm/llvm-project/blob/0aac9a2875bad4f065367e4a6553fad78605f895/clang/include/clang/Driver/Options.td#L5248-L5250
> >  | -target-cpu ]]
> Then why do they need aarch64-registered-target? Either they're coming from 
> the backend and so you need the backend, or they're coming from the frontend 
> and so you don't need the backend.
Can you clarify what you mean by "they"? I'm referring to driver's command line 
options, which is what this file is testing.

> Either they're coming from the backend and so you need the backend, or 
> they're coming from the frontend and so you don't need the backend.

Again, this is a driver test. The driver integrates the frontend and the 
backend (as well as other parts).

Are you suggesting that this test is moved to LLVM and a dependency on Flang is 
added to LLVM?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145883

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

[PATCH] D145883: [Flang][RISCV] Emit target features for RISC-V

[Andrzej Warzynski via Phabricator via cfe-commits]

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

LGTM, thanks for contributing!

Please wait for the other reviewers to confirm that they are happy with these 
changes.




Comment at: flang/test/Driver/target-cpu-features-invalid.f90:13
+! CHECK-INVALID-CPU: 'supercpu' is not a recognized processor for this target 
(ignoring processor)
+! CHECK-INVALID-FEATURE: '+superspeed' is not a recognized feature for this 
target (ignoring feature)

jrtc27 wrote:
> Don't these come from the backend? Testing them here doesn't seem right...
> Don't these come from the backend?

No. Both options are defined in Clang's Options.td:
* [[ 
https://github.com/llvm/llvm-project/blob/0aac9a2875bad4f065367e4a6553fad78605f895/clang/include/clang/Driver/Options.td#L5251-L5253
 | -target-feaure ]]
* [[ 
https://github.com/llvm/llvm-project/blob/0aac9a2875bad4f065367e4a6553fad78605f895/clang/include/clang/Driver/Options.td#L5248-L5250
 | -target-cpu ]]


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145883

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

[PATCH] D145883: [Flang][RISCV] Emit target features for RISC-V

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/test/Driver/code-gen-rv64.f90:7
+! RUN: %flang_fc1 -triple riscv64-unknown-linux-gnu \
+! RUN:   -target-feature +d -target-feature +c -emit-obj %s -o %t.o
+! RUN: llvm-readelf -h %t.o | FileCheck %s

jrtc27 wrote:
> Why do we need to go to an object file??? That's terrible practice in Clang 
> tests, and the same should be true of Flang. Test the IR, that is sufficient, 
> and decouples you from the backend.
> That's terrible practice in Clang tests, and the same should be true of 
> Flang. Test the IR, that is sufficient, and decouples you from the backend.

I disagree. 

A compiler driver is responsible for creating a correct backend/LLVM 
invocation. Is there some other way to verify that the backend invocation is 
correct? (other then inspecting the generated machine code file).

I agree that it is desirable to avoid any architectural details leaking outside 
of LLVM (into e.g. Clang and/or Flang), but IMHO it's very hard to avoid in 
practice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145883

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

[PATCH] D145883: [Flang][RISCV] Emit target features for RISC-V

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/test/Driver/code-gen-rv64.f90:12
+
+! CHECK: Flags: 0x5, RVC, double-float ABI
+end program

sunshaoce wrote:
> awarzynski wrote:
> > awarzynski wrote:
> > > For those of us less familiar with RISC-V - could you explain what's 
> > > significant about this line? For example, [[ 
> > > https://github.com/llvm/llvm-project/blob/0aac9a2875bad4f065367e4a6553fad78605f895/flang/test/Driver/code-gen-aarch64.f90#L18
> > >  | here ]] it is made clear that with the right triple used, one should 
> > > see a `ret` instruction within the main function (`_QQmain`). In here, I 
> > > just see a "magic" number :)
> > Follow-up questions - what's "Flags" and why "0x5"? Is there any online 
> > documentation that you can refer to here?
> You can refer: 
> https://github.com/riscv-non-isa/riscv-elf-psabi-doc/releases/download/v1.0/riscv-abi.pdf
>   `8.1. File Header` `e_flags` section
Thanks - and could you add a note in the test that explains where the "magic" 
0x5 comes from?



Comment at: flang/test/Driver/target-cpu-features.f90:33
 ! RUN: -o /dev/null -S %s 2>&1 | FileCheck %s -check-prefix=CHECK-INVALID-CPU
 
 ! RUN: %flang_fc1 -triple aarch64-linux-gnu -target-feature +superspeed \

sunshaoce wrote:
> Without `REQUIRES` line, test will be failed here.
> 
> Due to this patch has little to do with CPU-feature, I think we can only add 
> `code-gen-rv64.f90`. How about that?
This line verifies the driver diagnostics, i.e. it's very different to what 
other lines test in this file. You can safely move it to a dedicated file, e.g. 
"target-cpu-features-invalid.f90".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145883

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

[PATCH] D145883: [Flang][RISCV] Emit target features for RISC-V

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/test/Driver/target-cpu-features.f90:1
-! REQUIRES: aarch64-registered-target, x86-registered-target
+! REQUIRES: aarch64-registered-target, x86-registered-target, 
riscv-registered-target
 

vzakhari wrote:
> Can we split this test so that people who do not build riscv target still 
> have aarch64/x86 targets tested in their workspaces?
Given that this test relies on `-###`, I think that `REQUIRES` can be safely 
removed (please double check)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145883

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

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/test/Driver/omp-frontend-forwarding.f90:1
+! REQUIRES: amdgpu-registered-target
+

Given that you use `-###`, I think that this can be skipped (please double 
check).



Comment at: flang/test/Driver/omp-frontend-forwarding.f90:13
+! CHECK: "{{[^"]*}}flang-new" "-fc1" "-triple" "aarch64-unknown-linux-gnu" 
{{.*}} "-fopenmp" {{.*}}.f90"
+! CHECK-NOT: "{{[^"]*}}flang-new" "-fc1" "-triple" "aarch64-unknown-linux-gnu" 
{{.*}} "-fopenmp" {{.*}} "-fopenmp-is-device" {{.*}}.f90"
+! CHECK: "{{[^"]*}}flang-new" "-fc1" "-triple" "amdgcn-amd-amdhsa" {{.*}} 
"-fopenmp" {{.*}} "-fopenmp-is-device" {{.*}}.f90"

I feel that you can safely remove this line and replace the following with 
`CHECK-NEXT`, no? Just wondering whether there's a way to reduce the noise in 
these tests (without sacrificing the rigor).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145815

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

[PATCH] D145883: [Flang][RISCV] Emit target features for RISC-V

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/test/Driver/code-gen-rv64.f90:2
+! Test -emit-obj (RISC-V 64)
+
+! RUN: rm -f %t.o

Missing `REQUIRES: ` - this test won't work unless the RISC-V backend is 
available. 



Comment at: flang/test/Driver/code-gen-rv64.f90:12
+
+! CHECK: Flags: 0x5, RVC, double-float ABI
+end program

awarzynski wrote:
> For those of us less familiar with RISC-V - could you explain what's 
> significant about this line? For example, [[ 
> https://github.com/llvm/llvm-project/blob/0aac9a2875bad4f065367e4a6553fad78605f895/flang/test/Driver/code-gen-aarch64.f90#L18
>  | here ]] it is made clear that with the right triple used, one should see a 
> `ret` instruction within the main function (`_QQmain`). In here, I just see a 
> "magic" number :)
Follow-up questions - what's "Flags" and why "0x5"? Is there any online 
documentation that you can refer to here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145883

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

[PATCH] D145883: [Flang][RISCV] Emit target features for RISC-V

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/test/Driver/code-gen-rv64.f90:12
+
+! CHECK: Flags: 0x5, RVC, double-float ABI
+end program

For those of us less familiar with RISC-V - could you explain what's 
significant about this line? For example, [[ 
https://github.com/llvm/llvm-project/blob/0aac9a2875bad4f065367e4a6553fad78605f895/flang/test/Driver/code-gen-aarch64.f90#L18
 | here ]] it is made clear that with the right triple used, one should see a 
`ret` instruction within the main function (`_QQmain`). In here, I just see a 
"magic" number :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145883

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

[PATCH] D145883: [Flang][RISCV] Emit target features for RISC-V

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.
Herald added a subscriber: jobnoorman.

> Fix the issue of .o file generated by Flang with Flags info is 0x0 under 
> RISC-V.

TBH, I don't see how this is addressed in this patch. If that's something that 
this patch is intending to fix, then there should be a regression test added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145883

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

[PATCH] D145883: [Flang][RISCV] Emit target features for RISC-V

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/test/Driver/target-features.f90:1
+! RUN: %flang --target=riscv64-linux-gnu --target=riscv64 -c %s -### 2>&1 \
+! RUN: | FileCheck %s -check-prefix=CHECK-RV64

jrtc27 wrote:
> awarzynski wrote:
> > What happens if the RISC-V backend is not available?
> Clang doesn't need a backend to be available to generate IR for that 
> architecture. I would hope Flang is the same.
> Clang doesn't need a backend to be available to generate IR 

This test is not generating LLVM IR. It does, however, specify the target which 
is then translated into many **LLVM**-specific flags. 

Also:
```
clang --target=riscv64-linux-gnu --target=riscv64 -c file2.c
error: unable to create target: 'No available targets are compatible with 
triple "riscv64"'
1 error generated.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145883

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

[PATCH] D145883: [Flang][RISCV] Emit target features for RISC-V

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Why does "flang/test/Driver/target-features.f90" list all RISC-V features?  Why 
not use 
https://github.com/llvm/llvm-project/blob/main/flang/test/Driver/target-cpu-features.f90
 instead?




Comment at: flang/test/Driver/target-features.f90:1
+! RUN: %flang --target=riscv64-linux-gnu --target=riscv64 -c %s -### 2>&1 \
+! RUN: | FileCheck %s -check-prefix=CHECK-RV64

What happens if the RISC-V backend is not available?



Comment at: flang/test/Driver/target-features.f90:4
+
+! CHECK-RV64: "-target-feature" "+m" "-target-feature" "+a" "-target-feature" 
"+c" "-target-feature" "-e" "-target-feature" "-f" "-target-feature" "-d" 
"-target-feature" "-h" "-target-feature" "-zihintpause" "-target-feature" 
"-zfhmin" "-target-feature" "-zfh" "-target-feature" "-zfinx" "-target-feature" 
"-zdinx" "-target-feature" "-zhinxmin" "-target-feature" "-zhinx" 
"-target-feature" "-zba" "-target-feature" "-zbb" "-target-feature" "-zbc" 
"-target-feature" "-zbs" "-target-feature" "-zbkb" "-target-feature" "-zbkc" 
"-target-feature" "-zbkx" "-target-feature" "-zknd" "-target-feature" "-zkne" 
"-target-feature" "-zknh" "-target-feature" "-zksed" "-target-feature" "-zksh" 
"-target-feature" "-zkr" "-target-feature" "-zkn" "-target-feature" "-zks" 
"-target-feature" "-zkt" "-target-feature" "-zk" "-target-feature" "-zmmul" 
"-target-feature" "-v" "-target-feature" "-zvl32b" "-target-feature" "-zvl64b" 
"-target-feature" "-zvl128b" "-target-feature" "-zvl256b" "-target-feature" 
"-zvl512b" "-target-feature" "-zvl1024b" "-target-feature" "-zvl2048b" 
"-target-feature" "-zvl4096b" "-target-feature" "-zvl8192b" "-target-feature" 
"-zvl16384b" "-target-feature" "-zvl32768b" "-target-feature" "-zvl65536b" 
"-target-feature" "-zve32x" "-target-feature" "-zve32f" "-target-feature" 
"-zve64x" "-target-feature" "-zve64f" "-target-feature" "-zve64d" 
"-target-feature" "-zicbom" "-target-feature" "-zicboz" "-target-feature" 
"-zicbop" "-target-feature" "-zicsr" "-target-feature" "-zifencei" 
"-target-feature" "-zawrs" "-target-feature" "-svnapot" "-target-feature" 
"-svpbmt" "-target-feature" "-svinval" "-target-feature" "-xtheadba" 
"-target-feature" "-xtheadbb" "-target-feature" "-xtheadbs" "-target-feature" 
"-xtheadcmo" "-target-feature" "-xtheadcondmov" "-target-feature" 
"-xtheadfmemidx" "-target-feature" "-xtheadmac" "-target-feature" 
"-xtheadmemidx" "-target-feature" "-xtheadmempair" "-target-feature" 
"-xtheadsync" "-target-feature" "-xtheadvdot" "-target-feature" 
"-xventanacondops" "-target-feature" "-experimental-zihintntl" 
"-target-feature" "-experimental-zca" "-target-feature" "-experimental-zcb" 
"-target-feature" "-experimental-zcd" "-target-feature" "-experimental-zcf" 
"-target-feature" "-experimental-zfa" "-target-feature" "-experimental-zvfh" 
"-target-feature" "-experimental-ztso" "-target-feature" "+relax" 
"-target-feature" "-save-restore"

 This one line will require updating once respective bit of LLVM is updated. 
Let's avoid that :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145883

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

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

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski accepted this revision.
awarzynski 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/D145845/new/

https://reviews.llvm.org/D145845

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

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

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



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

This line will not appear without `-flang-experimental-exec` - you need to 
delete it. Same for the other test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145845

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