[PATCH] D142065: [SVE] Fix incorrect lowering of predicate permute builtins.

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added inline comments.



Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:6361
+  def : SVE_2_Op_Pat(NAME # 
_S)>;
+  def : SVE_2_Op_Pat(NAME # 
_D)>;
 }

Out of interest, is there a good reason to handle the nxv16 pattern case 
differently in the `I` multiclass args? Written this way at a glance it looks 
like it is missing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142065

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

[PATCH] D137239: [AArch64] Install arm_neon_sve_bridge.h

[Peter Waller via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8cb9e3c3ce1e: [AArch64] Install arm_neon_sve_bridge.h 
(authored by peterwaller-arm).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137239

Files:
  clang/lib/Headers/CMakeLists.txt


Index: clang/lib/Headers/CMakeLists.txt
===
--- clang/lib/Headers/CMakeLists.txt
+++ clang/lib/Headers/CMakeLists.txt
@@ -33,6 +33,7 @@
 
 set(aarch64_only_files
   arm64intr.h
+  arm_neon_sve_bridge.h
   )
 
 set(cuda_files
@@ -326,10 +327,6 @@
   clang_generate_header(-gen-arm-mve-header arm_mve.td arm_mve.h)
   # Generate arm_cde.h
   clang_generate_header(-gen-arm-cde-header arm_cde.td arm_cde.h)
-  # Copy arm_neon_sve_bridge.h
-  copy_header_to_output_dir(${CMAKE_CURRENT_SOURCE_DIR}
-arm_neon_sve_bridge.h
-  )
 
   # Add headers to target specific lists
   list(APPEND arm_common_generated_files
@@ -345,7 +342,6 @@
   list(APPEND aarch64_only_generated_files
 "${CMAKE_CURRENT_BINARY_DIR}/arm_sve.h"
 "${CMAKE_CURRENT_BINARY_DIR}/arm_bf16.h"
-"${output_dir}/arm_neon_sve_bridge.h"
 )
 endif()
 if(RISCV IN_LIST LLVM_TARGETS_TO_BUILD)


Index: clang/lib/Headers/CMakeLists.txt
===
--- clang/lib/Headers/CMakeLists.txt
+++ clang/lib/Headers/CMakeLists.txt
@@ -33,6 +33,7 @@
 
 set(aarch64_only_files
   arm64intr.h
+  arm_neon_sve_bridge.h
   )
 
 set(cuda_files
@@ -326,10 +327,6 @@
   clang_generate_header(-gen-arm-mve-header arm_mve.td arm_mve.h)
   # Generate arm_cde.h
   clang_generate_header(-gen-arm-cde-header arm_cde.td arm_cde.h)
-  # Copy arm_neon_sve_bridge.h
-  copy_header_to_output_dir(${CMAKE_CURRENT_SOURCE_DIR}
-arm_neon_sve_bridge.h
-  )
 
   # Add headers to target specific lists
   list(APPEND arm_common_generated_files
@@ -345,7 +342,6 @@
   list(APPEND aarch64_only_generated_files
 "${CMAKE_CURRENT_BINARY_DIR}/arm_sve.h"
 "${CMAKE_CURRENT_BINARY_DIR}/arm_bf16.h"
-"${output_dir}/arm_neon_sve_bridge.h"
 )
 endif()
 if(RISCV IN_LIST LLVM_TARGETS_TO_BUILD)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137239: [AArch64] Install arm_neon_sve_bridge.h

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm created this revision.
peterwaller-arm added reviewers: MattDevereau, benmxwl-arm, c-rhodes.
Herald added subscribers: ctetreau, kristof.beyls, tschuett.
Herald added a project: All.
peterwaller-arm requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

arm_neon_sve_bridge.h is not generated, so the rules which ensure the
generated files get copied into the installation prefix don't apply to
this one.

Add it to the aarch64_only_files set instead, which ensures it ends up
both in the build directory and the installation directory.

Tested with build targets `clang-resource-headers` and
`install-clang-resource-headers`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D137239

Files:
  clang/lib/Headers/CMakeLists.txt


Index: clang/lib/Headers/CMakeLists.txt
===
--- clang/lib/Headers/CMakeLists.txt
+++ clang/lib/Headers/CMakeLists.txt
@@ -33,6 +33,7 @@
 
 set(aarch64_only_files
   arm64intr.h
+  arm_neon_sve_bridge.h
   )
 
 set(cuda_files
@@ -326,10 +327,6 @@
   clang_generate_header(-gen-arm-mve-header arm_mve.td arm_mve.h)
   # Generate arm_cde.h
   clang_generate_header(-gen-arm-cde-header arm_cde.td arm_cde.h)
-  # Copy arm_neon_sve_bridge.h
-  copy_header_to_output_dir(${CMAKE_CURRENT_SOURCE_DIR}
-arm_neon_sve_bridge.h
-  )
 
   # Add headers to target specific lists
   list(APPEND arm_common_generated_files
@@ -345,7 +342,6 @@
   list(APPEND aarch64_only_generated_files
 "${CMAKE_CURRENT_BINARY_DIR}/arm_sve.h"
 "${CMAKE_CURRENT_BINARY_DIR}/arm_bf16.h"
-"${output_dir}/arm_neon_sve_bridge.h"
 )
 endif()
 if(RISCV IN_LIST LLVM_TARGETS_TO_BUILD)


Index: clang/lib/Headers/CMakeLists.txt
===
--- clang/lib/Headers/CMakeLists.txt
+++ clang/lib/Headers/CMakeLists.txt
@@ -33,6 +33,7 @@
 
 set(aarch64_only_files
   arm64intr.h
+  arm_neon_sve_bridge.h
   )
 
 set(cuda_files
@@ -326,10 +327,6 @@
   clang_generate_header(-gen-arm-mve-header arm_mve.td arm_mve.h)
   # Generate arm_cde.h
   clang_generate_header(-gen-arm-cde-header arm_cde.td arm_cde.h)
-  # Copy arm_neon_sve_bridge.h
-  copy_header_to_output_dir(${CMAKE_CURRENT_SOURCE_DIR}
-arm_neon_sve_bridge.h
-  )
 
   # Add headers to target specific lists
   list(APPEND arm_common_generated_files
@@ -345,7 +342,6 @@
   list(APPEND aarch64_only_generated_files
 "${CMAKE_CURRENT_BINARY_DIR}/arm_sve.h"
 "${CMAKE_CURRENT_BINARY_DIR}/arm_bf16.h"
-"${output_dir}/arm_neon_sve_bridge.h"
 )
 endif()
 if(RISCV IN_LIST LLVM_TARGETS_TO_BUILD)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131573: [clang][AArch64][SVE] Change SVE_VECTOR_OPERATORS macro for VLA vectors

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm accepted this revision.
peterwaller-arm added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/Basic/Targets/AArch64.cpp:493
+  // C/C++ operators work on both VLS and VLA SVE types
+  if (FPU & SveMode)
+Builder.defineMacro("__ARM_FEATURE_SVE_VECTOR_OPERATORS", "2");

peterwaller-arm wrote:
> Wrong operator?
Sorry, I see I misinterpreted this, it's a bitmask check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131573

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

[PATCH] D131573: [clang][AArch64][SVE] Change SVE_VECTOR_OPERATORS macro for VLA vectors

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm requested changes to this revision.
peterwaller-arm added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/Basic/Targets/AArch64.cpp:493
+  // C/C++ operators work on both VLS and VLA SVE types
+  if (FPU & SveMode)
+Builder.defineMacro("__ARM_FEATURE_SVE_VECTOR_OPERATORS", "2");

Wrong operator?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131573

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

[PATCH] D129135: [doc][ReleaseNotes] Document AArch64 SVE ABI fix from D127209

[Peter Waller 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 rG2db2a4e11240: [doc][ReleaseNotes] Document AArch64 SVE ABI 
fix from D127209 (authored by peterwaller-arm).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129135

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -503,6 +503,14 @@
   (e.g. ``int : 0``) no longer prevents the structure from being considered a
   homogeneous floating-point or vector aggregate. The new behavior agrees with
   the AAPCS specification, and matches the similar bug fix in GCC 12.1.
+- Targeting AArch64, since D127209 LLVM now only preserves the z8-z23
+  and p4-p15 registers across a call if the registers z0-z7 or p0-p3 are
+  used to pass data into or out of a subroutine. The new behavior
+  matches the AAPCS. Previously LLVM preserved z8-z23 and p4-p15 across
+  a call if the callee had an SVE type anywhere in its signature. This
+  would cause an incorrect use of the caller-preserved z8-z23 and p4-p15
+  ABI for example if the 9th argument or greater were the first SVE type
+  in the signature of a function.
 - All copy constructors can now be trivial if they are not user-provided,
   regardless of the type qualifiers of the argument of the defaulted 
constructor,
   fixing dr2171.


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -503,6 +503,14 @@
   (e.g. ``int : 0``) no longer prevents the structure from being considered a
   homogeneous floating-point or vector aggregate. The new behavior agrees with
   the AAPCS specification, and matches the similar bug fix in GCC 12.1.
+- Targeting AArch64, since D127209 LLVM now only preserves the z8-z23
+  and p4-p15 registers across a call if the registers z0-z7 or p0-p3 are
+  used to pass data into or out of a subroutine. The new behavior
+  matches the AAPCS. Previously LLVM preserved z8-z23 and p4-p15 across
+  a call if the callee had an SVE type anywhere in its signature. This
+  would cause an incorrect use of the caller-preserved z8-z23 and p4-p15
+  ABI for example if the 9th argument or greater were the first SVE type
+  in the signature of a function.
 - All copy constructors can now be trivial if they are not user-provided,
   regardless of the type qualifiers of the argument of the defaulted constructor,
   fixing dr2171.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129135: [doc][ReleaseNotes] Document AArch64 SVE ABI fix from D127209

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm updated this revision to Diff 442264.
peterwaller-arm added a comment.

s/p0-p4/p0-p3/, thanks for the keen eye, rsandifo-arm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129135

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -499,6 +499,14 @@
   (e.g. ``int : 0``) no longer prevents the structure from being considered a
   homogeneous floating-point or vector aggregate. The new behavior agrees with
   the AAPCS specification, and matches the similar bug fix in GCC 12.1.
+- Targeting AArch64, since D127209 LLVM now only preserves the z8-z23
+  and p4-p15 registers across a call if the registers z0-z7 or p0-p3 are
+  used to pass data into or out of a subroutine. The new behavior
+  matches the AAPCS. Previously LLVM preserved z8-z23 and p4-p15 across
+  a call if the callee had an SVE type anywhere in its signature. This
+  would cause an incorrect use of the caller-preserved z8-z23 and p4-p15
+  ABI for example if the 9th argument or greater were the first SVE type
+  in the signature of a function.
 - All copy constructors can now be trivial if they are not user-provided,
   regardless of the type qualifiers of the argument of the defaulted 
constructor,
   fixing dr2171.


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -499,6 +499,14 @@
   (e.g. ``int : 0``) no longer prevents the structure from being considered a
   homogeneous floating-point or vector aggregate. The new behavior agrees with
   the AAPCS specification, and matches the similar bug fix in GCC 12.1.
+- Targeting AArch64, since D127209 LLVM now only preserves the z8-z23
+  and p4-p15 registers across a call if the registers z0-z7 or p0-p3 are
+  used to pass data into or out of a subroutine. The new behavior
+  matches the AAPCS. Previously LLVM preserved z8-z23 and p4-p15 across
+  a call if the callee had an SVE type anywhere in its signature. This
+  would cause an incorrect use of the caller-preserved z8-z23 and p4-p15
+  ABI for example if the 9th argument or greater were the first SVE type
+  in the signature of a function.
 - All copy constructors can now be trivial if they are not user-provided,
   regardless of the type qualifiers of the argument of the defaulted constructor,
   fixing dr2171.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129135: [doc][ReleaseNotes] Document AArch64 SVE ABI fix from D127209

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm updated this revision to Diff 442253.
peterwaller-arm marked an inline comment as done.
peterwaller-arm edited the summary of this revision.
peterwaller-arm added a comment.

- Update commit message: "This affects" -> "The fix affects"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129135

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -499,6 +499,14 @@
   (e.g. ``int : 0``) no longer prevents the structure from being considered a
   homogeneous floating-point or vector aggregate. The new behavior agrees with
   the AAPCS specification, and matches the similar bug fix in GCC 12.1.
+- Targeting AArch64, since D127209 LLVM now only preserves the z8-z23
+  and p4-p15 registers across a call if the registers z0-z7 or p0-p4 are
+  used to pass data into or out of a subroutine. The new behavior
+  matches the AAPCS. Previously LLVM preserved z8-z23 and p4-p15 across
+  a call if the callee had an SVE type anywhere in its signature. This
+  would cause an incorrect use of the caller-preserved z8-z23 and p4-p15
+  ABI for example if the 9th argument or greater were the first SVE type
+  in the signature of a function.
 - All copy constructors can now be trivial if they are not user-provided,
   regardless of the type qualifiers of the argument of the defaulted 
constructor,
   fixing dr2171.


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -499,6 +499,14 @@
   (e.g. ``int : 0``) no longer prevents the structure from being considered a
   homogeneous floating-point or vector aggregate. The new behavior agrees with
   the AAPCS specification, and matches the similar bug fix in GCC 12.1.
+- Targeting AArch64, since D127209 LLVM now only preserves the z8-z23
+  and p4-p15 registers across a call if the registers z0-z7 or p0-p4 are
+  used to pass data into or out of a subroutine. The new behavior
+  matches the AAPCS. Previously LLVM preserved z8-z23 and p4-p15 across
+  a call if the callee had an SVE type anywhere in its signature. This
+  would cause an incorrect use of the caller-preserved z8-z23 and p4-p15
+  ABI for example if the 9th argument or greater were the first SVE type
+  in the signature of a function.
 - All copy constructors can now be trivial if they are not user-provided,
   regardless of the type qualifiers of the argument of the defaulted constructor,
   fixing dr2171.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129135: [doc][ReleaseNotes] Document AArch64 SVE ABI fix from D127209

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm marked an inline comment as done.
peterwaller-arm added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:503
+- Targeting AArch64, LLVM now only preserves the z8-z23 registers across
+  a call if the registers z0-z7 are used to pass data into or out of a
+  subroutine. This new behavior now matches the AAPCS. Previously LLVM

rsandifo-arm wrote:
> Pedantically, I think it should be “the registers z0-z7 or p0-p3”.
I've now switched to writing out all of the registers every time, hope that's 
clearer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129135

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

[PATCH] D129135: [doc][ReleaseNotes] Document AArch64 SVE ABI fix from D127209

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm updated this revision to Diff 442251.
peterwaller-arm added a comment.

- Cite D127209  in the release note per 
tschuett comment.
- Be more specific about p0-p4, remove 'by analogy', per rsandifo-arm comment.
- Clarify 9th argument or greater, per DavidSpicket comment.

Thank you everyone for your inputs, hope this addresses everything so far, 
happy to receive further input.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129135

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -499,6 +499,14 @@
   (e.g. ``int : 0``) no longer prevents the structure from being considered a
   homogeneous floating-point or vector aggregate. The new behavior agrees with
   the AAPCS specification, and matches the similar bug fix in GCC 12.1.
+- Targeting AArch64, since D127209 LLVM now only preserves the z8-z23
+  and p4-p15 registers across a call if the registers z0-z7 or p0-p4 are
+  used to pass data into or out of a subroutine. The new behavior
+  matches the AAPCS. Previously LLVM preserved z8-z23 and p4-p15 across
+  a call if the callee had an SVE type anywhere in its signature. This
+  would cause an incorrect use of the caller-preserved z8-z23 and p4-p15
+  ABI for example if the 9th argument or greater were the first SVE type
+  in the signature of a function.
 - All copy constructors can now be trivial if they are not user-provided,
   regardless of the type qualifiers of the argument of the defaulted 
constructor,
   fixing dr2171.


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -499,6 +499,14 @@
   (e.g. ``int : 0``) no longer prevents the structure from being considered a
   homogeneous floating-point or vector aggregate. The new behavior agrees with
   the AAPCS specification, and matches the similar bug fix in GCC 12.1.
+- Targeting AArch64, since D127209 LLVM now only preserves the z8-z23
+  and p4-p15 registers across a call if the registers z0-z7 or p0-p4 are
+  used to pass data into or out of a subroutine. The new behavior
+  matches the AAPCS. Previously LLVM preserved z8-z23 and p4-p15 across
+  a call if the callee had an SVE type anywhere in its signature. This
+  would cause an incorrect use of the caller-preserved z8-z23 and p4-p15
+  ABI for example if the 9th argument or greater were the first SVE type
+  in the signature of a function.
 - All copy constructors can now be trivial if they are not user-provided,
   regardless of the type qualifiers of the argument of the defaulted constructor,
   fixing dr2171.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129135: [doc][ReleaseNotes] Document AArch64 SVE ABI fix from D127209

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

In D129135#3629860 , @peterwaller-arm 
wrote:

> but I can't identify a specific worse issue at present.

Thinking a little harder, one breaking case is where a 
callee-compiled-with-wrong-logic looks at its signature, concludes it does not 
need to preserve the regs (because the SVE type is in the 9th argument, for 
example), but is called by a different compiler which correctly implements the 
ABI, so neither side preserves registers which require preservation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129135

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

[PATCH] D129135: [doc][ReleaseNotes] Document AArch64 SVE ABI fix from D127209

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

In D129135#3629833 , @DavidSpickett 
wrote:

> From reading the release note my understanding is that before this fix the 
> caller of a function would store `z0-z7` in situations where it did not need 
> to.

I believe you are correct, with an inkling of doubt in the back of my mind, but 
I can't identify a specific worse issue at present.

> Wouldn't the type of the preceding 8 arguments also be relevant?

My intent was to express that the issue is present where it's the 9th or later 
arguments.

> This is because the function would return in `z0` therefore the caller must 
> preserve at least `z0` and the ABI tells you to preserve `z0-z7` to do that?

Yes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129135

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

[PATCH] D129135: [doc][ReleaseNotes] Document AArch64 SVE ABI fix from D127209

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm created this revision.
peterwaller-arm added reviewers: rsandifo-arm, kristof.beyls.
Herald added subscribers: ctetreau, tschuett.
Herald added a project: All.
peterwaller-arm requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

D127209  fixed LLVM to bring it in line with 
the AAPCS. This affects
functions where the first SVE parameter appears in the 9th or later
arguments, and the function does not return an SVE type.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129135

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -499,6 +499,15 @@
   (e.g. ``int : 0``) no longer prevents the structure from being considered a
   homogeneous floating-point or vector aggregate. The new behavior agrees with
   the AAPCS specification, and matches the similar bug fix in GCC 12.1.
+- Targeting AArch64, LLVM now only preserves the z8-z23 registers across
+  a call if the registers z0-z7 are used to pass data into or out of a
+  subroutine. This new behavior now matches the AAPCS. Previously LLVM
+  preserved z8-z23 across a call if the callee had an SVE type anywhere
+  in its signature. This would cause an incorrect use of the
+  caller-preserved z8-z23 ABI for example if the 9th argument to a
+  function were an SVE type. The analogous issue and fix applies to
+  predicate register arguments (p0-p3 for passing between subroutines,
+  and p4-15 preserved).
 - All copy constructors can now be trivial if they are not user-provided,
   regardless of the type qualifiers of the argument of the defaulted 
constructor,
   fixing dr2171.


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -499,6 +499,15 @@
   (e.g. ``int : 0``) no longer prevents the structure from being considered a
   homogeneous floating-point or vector aggregate. The new behavior agrees with
   the AAPCS specification, and matches the similar bug fix in GCC 12.1.
+- Targeting AArch64, LLVM now only preserves the z8-z23 registers across
+  a call if the registers z0-z7 are used to pass data into or out of a
+  subroutine. This new behavior now matches the AAPCS. Previously LLVM
+  preserved z8-z23 across a call if the callee had an SVE type anywhere
+  in its signature. This would cause an incorrect use of the
+  caller-preserved z8-z23 ABI for example if the 9th argument to a
+  function were an SVE type. The analogous issue and fix applies to
+  predicate register arguments (p0-p3 for passing between subroutines,
+  and p4-15 preserved).
 - All copy constructors can now be trivial if they are not user-provided,
   regardless of the type qualifiers of the argument of the defaulted constructor,
   fixing dr2171.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124860: [clang][AArch64][SVE] Implicit conversions for vector-scalar operations

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:10586
+  (!LHSBuiltinTy && !LHSBuiltinTy->isVLSTBuiltinType() &&
+   !LHSType->isRealType())) {
+Diag(Loc, diag::err_typecheck_vector_not_convertable_non_scalar)

`!Ptr && [Deref Ptr]` looks suspicious. Also, if I insert an abort in this path 
and run check-clang I don't see any failing tests?

It might help to name `LHSBuiltinTy && LHSBuiltinTy->isVLSTBuiltinType()` 
`IsLHSVector` (likewise for RHS) or similar.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124860

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

[PATCH] D124836: [AArch64] Add support for -fzero-call-used-regs

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added inline comments.



Comment at: llvm/test/CodeGen/AArch64/zero-call-used-regs.ll:259-262
+; SVE-NEXT:pfalse p0.b
+; SVE-NEXT:pfalse p1.b
+; SVE-NEXT:pfalse p2.b
+; SVE-NEXT:pfalse p3.b

nickdesaulniers wrote:
> N00b question about SVE: do we need `pfalse` for each of the numbered p 
> registers corresponding to the x registers we zeroed? i.e. here we have 
> pfalse for p0-3, yet we zero z0-7.
No, the set of p registers are independent of the z registers.

The calling convention states [0] that the predicate registers p0-p3 may be 
used for parameter passing (if you have an argument which belongs in a p 
register), so this looks reasonable.

[0] 
https://github.com/ARM-software/abi-aa/blob/8a7b266879c60ca1c76e94ebb279b2dac60ed6a5/aapcs64/aapcs64.rst#scalable-predicate-registers


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124836

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

[PATCH] D124860: [clang][AArch64][SVE] Implicit conversions for vector-scalar operations

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

Looking pretty good. A couple of test cases to consider:

  #include 
  #include 
  svint8_t svi8(svint8_t a) {
return a + 256;
  }
  int8x16_t nei8(int8x16_t a) {
  return a + 256;
  }
  svint8_t svi8_128(svint8_t a) {
return a + 128;
  }
  int8x16_t nei8_128(int8x16_t a) {
  return a + 128;
  }

  inp.c:4:13: error: invalid operands to binary expression ('svint8_t' (aka 
'__SVInt8_t') and 'int')
return a + 256;
   ~ ^ ~~~
  inp.c:7:11: error: cannot convert between scalar type 'int' and vector type 
'int8x16_t' (vector of 16 'int8_t' values) as implicit conversion would cause 
truncation
  return a + 256;
   ^
  inp.c:13:13: warning: implicit conversion from 'int' to 'int8x16_t' (vector 
of 16 'int8_t' values) changes value from 128 to -128 [-Wconstant-conversion]
  return a + 128;
   ~ ^~~

Note that for NEON we get a warning about the implicit conversion, and a clear 
error message about truncation, but for SVE there is no warning and simply an 
'invalid operands to binary expression', which could be confusing since the 
implicit conversions exist for the same operand type with differing value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124860

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

[PATCH] D124836: [AArch64] Add support for -fzero-call-used-regs

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added inline comments.



Comment at: llvm/test/CodeGen/AArch64/zero-call-used-regs.ll:233
+
+attributes #0 = { mustprogress nofree norecurse nosync nounwind readnone 
willreturn uwtable "frame-pointer"="non-leaf" "min-legal-vector-width"="0" 
"no-trapping-math"="true" "stack-protector-buffer-size"="8" 
"target-cpu"="generic" "target-features"="+neon,+v8a" }

Thanks for addressing the SVE case. Please can we have `target-features=+sve` 
tests as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124836

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

[PATCH] D124998: [AArch64][SVE] Add aarch64_sve_pcs attribute to Clang

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added inline comments.



Comment at: clang/include/clang-c/Index.h:3448
+  CXCallingConv_AArch64SVEPcs= 17,
+  CXCallingConv_SwiftAsync = 18,
 

peterwaller-arm wrote:
> It shouldn't matter in principle (... "but in practice" ...) we should 
> probably avoid renumbering existing things in the enum and instead add to the 
> end of it.
> 
> Nit, this is missing a space before the equals.
> Nit, SVE is an acronym, so is PCS, so capitalization should be consistent 
> between the two. I see 'PCS' capitalized in AAPCS for example so probably all 
> upper case makes the sense.
> 
I retract my sloppy "it shouldn't matter in principle [at the source level]", 
of course it does matter, and it likely matters in this case (see 'alias for 
compatibility' comment above).

To be more specific, changing the enum is an ABI break, and breaks if these 
things are ever serialized and therefore not something you want to do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124998

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

[PATCH] D124998: [AArch64][SVE] Add aarch64_sve_pcs attribute to Clang

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm accepted this revision.
peterwaller-arm added a comment.
This revision is now accepted and ready to land.

Looks good to me with minor nits.




Comment at: clang/include/clang-c/Index.h:3448
+  CXCallingConv_AArch64SVEPcs= 17,
+  CXCallingConv_SwiftAsync = 18,
 

It shouldn't matter in principle (... "but in practice" ...) we should probably 
avoid renumbering existing things in the enum and instead add to the end of it.

Nit, this is missing a space before the equals.
Nit, SVE is an acronym, so is PCS, so capitalization should be consistent 
between the two. I see 'PCS' capitalized in AAPCS for example so probably all 
upper case makes the sense.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124998

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

[PATCH] D124836: [AArch64] Add support for -fzero-call-used-regs

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added inline comments.



Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:752
+#undef CASE
+  }
+}

void wrote:
> kristof.beyls wrote:
> > Just a drive-by comment: I'm wondering if SVE registers should also be 
> > listed here?
> I'm not familiar with the SVE registers (I assume you mean the `Z#` and `P#` 
> ones). Could you give an example program?
SVE is slightly tricker here because the set of registers the caller must 
preserve depends on the signature of the function.

This is described here: 
https://github.com/ARM-software/abi-aa/blob/8a7b266879c60ca1c76e94ebb279b2dac60ed6a5/aapcs64/aapcs64.rst#613scalable-vector-registers

The callee-preserved registers are z8-z23 and p4-p15 if the function is using 
the VARIANT_PCS, the code for that condition in the asm printer is here:

https://github.com/llvm/llvm-project/blob/78fd413cf736953ac623cabf3d5f84c8219e31f8/llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp#L864-L875


```
 if (MF->getFunction().getCallingConv() == CallingConv::AArch64_VectorCall ||
  MF->getFunction().getCallingConv() ==
  CallingConv::AArch64_SVE_VectorCall ||
  STI->getRegisterInfo()->hasSVEArgsOrReturn(MF)) {
```

Hope that helps a little.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124836

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

[PATCH] D124091: [clang][AArch64][SVE] Implement conditional operator for SVE vectors

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added inline comments.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:6266
+LHS = ImpCastExprToType(LHS.get(), ResultType, CK_VectorSplat);
+RHS = ImpCastExprToType(RHS.get(), ResultType, CK_VectorSplat);
+  }

I get an assert with the following input (float64 in, float32 out). I was lead 
here by noticing that the element count comes from `CondElementCount`, but 
nothing ensures that the numbers of elements are consistent with LHS or RHS.

```
svfloat32_t cond_f32_splat(svfloat64_t a) {
return a < 0. ? 1.f : 0.f;
}
```

```
#11 0x01fef77e clang::ASTContext::getCanonicalType(clang::QualType) 
const CodeGenTypes.cpp:0:0
#12 0x0446b39c clang::Sema::ImpCastExprToType(clang::Expr*, 
clang::QualType, clang::CastKind, clang::ExprValueKind, 
llvm::SmallVector const*, clang::Sema::CheckedCo
nversionKind) (out/rel/bin/clang+0x446b39c)
#13 0x048ca1ce 
clang::Sema::CheckSizelessVectorConditionalTypes(clang::ActionResult&, clang::ActionResult&, 
clang::ActionResult&, clan
g::SourceLocation) (out/rel/bin/clang+0x48ca1ce)
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124091

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

[PATCH] D122404: [clang][AArc64][SVE] Add support for comparison operators on SVE types

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm accepted this revision.
peterwaller-arm added a comment.
This revision is now accepted and ready to land.

LGTM, looks like the formatter might have hit some lines you didn't intend to 
change -- if it's not too invasive perhaps worth a seperate NFC patch to format 
the file?




Comment at: clang/lib/Sema/SemaExpr.cpp:13080-13086
+<< RHS.get()->getSourceRange() << (Opc == BO_LAnd ? "&&" : "||");
 // Suggest replacing the logical operator with the bitwise version
 Diag(Loc, diag::note_logical_instead_of_bitwise_change_operator)
 << (Opc == BO_LAnd ? "&" : "|")
-<< FixItHint::CreateReplacement(SourceRange(
- Loc, 
getLocForEndOfToken(Loc)),
-Opc == BO_LAnd ? "&" : "|");
+<< FixItHint::CreateReplacement(
+   SourceRange(Loc, getLocForEndOfToken(Loc)),
+   Opc == BO_LAnd ? "&" : "|");

Unintended whitespace changes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122404

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

[PATCH] D121792: [AArch64][SVE] InstCombine llvm.aarch64.sve.sel to select

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

In D121792#3388585 , @dmgreen wrote:

> Why do we have llvm.aarch64.sve.sel if we are always going to replace it with 
> a select? Why not remove llvm.aarch64.sve.sel entirely?

I'm not the key decisionmaker here, but hopefully I can summarize the position 
and others can chime in if I'm mistaken or misdirecting.

1. You're right, and we've been discussing the possibility removing these 
intrinsics where they're unnecessary.
2. The intrinsics exist largely to support the ACLE.
3. There are a significant number of them, at the moment there is a 
straightforward correspondence between the C level and IR level.
4. Because of the correspondence, there is currently very little 
code/complexity per-intrinsic required in clang to support the ACLE.
5. Therefore, removing these intrinsics requires increasing the complexity of 
the frontend.
6. Some intrinsics will always be required.

Given these considerations, our current thinking is to have a future effort to 
see if removing intrinsics like this and lowering directly to clean IR is a net 
win in total. And to commit to doing this with a holistic view of the situation 
rather than doing it bit-by-bit and making a mess.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121792

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

[PATCH] D121119: [clang][SVE] Add support for bitwise operators on SVE types

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm accepted this revision.
peterwaller-arm added a comment.
This revision is now accepted and ready to land.

LGTM, modulo that the require lines look like they need fixing.




Comment at: clang/test/CodeGen/aarch64-sve-vector-bitwise-ops.c:7
+
+// AND
+

Needs a requires line?



Comment at: clang/test/Sema/aarch64-sve-vector-bitwise-ops.c:2-3
+// RUN: %clang_cc1 -verify -triple aarch64-none-linux-gnu -target-feature +sve 
-fallow-half-arguments-and-returns -fsyntax-only %s
+
+// REQUIRES: aarch64-registered-target || arm-registered-target
+




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121119

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

[PATCH] D121119: [clang][SVE] Add support for bitwise operators on SVE types

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

Looking reasonable to me, one comment.




Comment at: clang/lib/Sema/SemaExpr.cpp:10404
+   SourceLocation Loc,
+   bool IsArithmetic) {
   QualType LHSType = LHS.get()->getType().getUnqualifiedType();

There is a saying that booleans are often inferior to enums in arguments. I 
think this might be one of those cases, could you use the `enum ArithConvKind` 
instead?

As a side effect call sites will be clear without the need for a comment string:
```
CheckSizelessVectorOperands(LHS, RHS, Loc, ACK_Arithmetic);
```

... and it will be extensible to other kinds, which I presume are likely to 
creep in here as more operators are added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121119

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

[PATCH] D119926: [Clang][AArch64] Enable _Float16 _Complex type

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added inline comments.



Comment at: clang/test/CodeGen/aarch64-complex-half-math.c:3
+// RUN: %clang_cc1 %s -emit-llvm -triple aarch64-unknown-unknown -o - | 
FileCheck %s --check-prefix=AARCH64
+// REQUIRES: aarch64-registered-target
+

aaron.ballman wrote:
> There's nothing AArch64-specific about the change, so I'd expect some more 
> general codegen tests.
> 
> Actually, I took a look to see what calls 
> `getFloatingTypeOfSizeWithinDomain()` and I can't find any callers of that. 
> Do you have a test case where you were hitting that particular unreachable?
Thanks for your observation @aaron.ballman.

It appears this has been working since D105331, which enabled it for X86.

Unless I'm missing something, the function in question appears to have been 
unused since 2010 (the last use was removed in 
d005ac937e4c08e0c607ddc42708f8bf2064c243 by @rjmccall), I presume 
`getFloatingTypeOfSizeWithinDomain` should be removed then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119926

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

[PATCH] D119926: [Clang][AArch64] Enable _Float16 _Complex type

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm accepted this revision.
peterwaller-arm added a comment.
This revision is now accepted and ready to land.

LGTM.




Comment at: clang/test/CodeGen/aarch64-complex-half-math.c:5
+  // AARCH64-LABEL: @add_float_rr(
+  // AARCH64: fadd reassoc nnan ninf nsz arcp afn half
+  // AARCH64-NOT: fadd

peterwaller-arm wrote:
> My read is that this appears to be testing both -ffast-math and _Float16. Do 
> we want to be testing both these things at once like this, can anyone comment?
> 
> My preference would be to have these test only the operators, and not the 
> flags, and then maybe have a single test which does
> https://clang.llvm.org/docs/LanguageExtensions.html#extensions-to-specify-floating-point-flags
> ```
> #pragma clang fp contract(fast)
> ```
> 
> and verifies that the flags are present.
> 
Discussed off-review: concluded for now that a no-optimizations test with no 
fast math flags seems reasonable, to address @fhahn's comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119926

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

[PATCH] D119926: [Clang][AArch64] Enable _Float16 _Complex type

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

Some comments.




Comment at: clang/include/clang/AST/ASTContext.h:1117
   CanQualType BFloat16Ty;
-  CanQualType Float16Ty; // C11 extension ISO/IEC TS 18661-3
+  CanQualType Float16Ty, Float16ComplexTy; // C11 extension ISO/IEC TS 18661-3
   CanQualType VoidPtrTy, NullPtrTy;

Is this necessary?



Comment at: clang/lib/AST/ASTContext.cpp:1330
   InitBuiltinType(Float16Ty,   BuiltinType::Float16);
+  InitBuiltinType(Float16ComplexTy,BuiltinType::Float16);
 

Is this necessary?



Comment at: clang/lib/AST/ASTContext.cpp:6811-6812
 switch (EltRank) {
-case BFloat16Rank: llvm_unreachable("Complex bfloat16 is not supported");
-case Float16Rank:
+case BFloat16Rank:
+  llvm_unreachable("Complex bfloat16 is not supported");
 case HalfRank: llvm_unreachable("Complex half is not supported");

This can be left unmodified.



Comment at: clang/test/CodeGen/aarch64-complex-half-math.c:1
+// RUN: %clang_cc1 %s -O1 -emit-llvm -triple aarch64-unknown-unknown 
-ffast-math -o - | FileCheck %s --check-prefix=AARCH64
+

Can these test outputs be autogenerated with update_cc_test_checks.py?



Comment at: clang/test/CodeGen/aarch64-complex-half-math.c:2
+// RUN: %clang_cc1 %s -O1 -emit-llvm -triple aarch64-unknown-unknown 
-ffast-math -o - | FileCheck %s --check-prefix=AARCH64
+
+_Float16 _Complex add_float_rr(_Float16 a, _Float16 b) {

I think this test needs a `// REQUIRES: aarch64-registered-target` to protect 
builds which don't have a LLVM_TARGETS_TO_BUILD which implies AArch64 is 
available.



Comment at: clang/test/CodeGen/aarch64-complex-half-math.c:5
+  // AARCH64-LABEL: @add_float_rr(
+  // AARCH64: fadd reassoc nnan ninf nsz arcp afn half
+  // AARCH64-NOT: fadd

My read is that this appears to be testing both -ffast-math and _Float16. Do we 
want to be testing both these things at once like this, can anyone comment?

My preference would be to have these test only the operators, and not the 
flags, and then maybe have a single test which does
https://clang.llvm.org/docs/LanguageExtensions.html#extensions-to-specify-floating-point-flags
```
#pragma clang fp contract(fast)
```

and verifies that the flags are present.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119926

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

[PATCH] D114713: [AArch64][SVE][NEON] Add NEON-SVE-Bridge intrinsics

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm accepted this revision.
peterwaller-arm added a comment.
This revision is now accepted and ready to land.

LGTM once D115259  has landed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114713

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

[PATCH] D114713: [AArch64][SVE][NEON] Add NEON-SVE-Bridge intrinsics

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added inline comments.



Comment at: clang/include/clang/Basic/BuiltinsAArch64NeonSVEBridge_cg.def:37
+SVEMAP2(svdup_neonq_f64, 3),
+SVEMAP2(svdup_neonq_bf16, 1),
+#endif

The second argument is a 'flags' field and these values don't look right.

Refs:

* [[ 
https://github.com/llvm/llvm-project/blob/84b978da3b80b986327a830c01e32f12cefe86b3/clang/utils/TableGen/SveEmitter.cpp#L1339
 | SVEMAP2 emitted by tablegen ]]
* [[ 
https://github.com/llvm/llvm-project/blob/84b978da3b80b986327a830c01e32f12cefe86b3/clang/include/clang/Basic/arm_sve.td#L137-L149
 | Element types ]]
* [[ 
https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGBuiltin.cpp#L9070
 | Flags used in CGBuiltin ]]

The flags are a generated enum and live in 
`clang/include/clang/Basic/arm_sve_typeflags.inc` -- I think you'll need to 
#include this with `LLVM_GET_SVE_ELTTYPES` defined, and then you can write it 
symbolically rather than using a literal numeric value.


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

https://reviews.llvm.org/D114713

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

[PATCH] D114713: [AArch64][SVE][NEON] Add NEON-SVE-Bridge intrinsics

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added inline comments.



Comment at: clang/include/clang/Basic/BuiltinsAArch64NeonSVEBridge.def:2
+#ifdef GET_SVE_BUILTINS
+BUILTIN(__builtin_sve_svget_neonq_s8, "q4iq16bq4i", "n")
+BUILTIN(__builtin_sve_svget_neonq_s16, "q4iq16bq4i", "n")

Looks like these aren't correct, the number should indicate the number of 
lanes, and it should be 16 for an s8. `q` indicates scalable, which is also not 
right for this builtin.

The code which parses these strings can be found here:

https://github.com/llvm/llvm-project/blob/14c4051122bf4070d624b82189f1093758ecdf69/clang/lib/AST/ASTContext.cpp#L10417


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114713

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

[PATCH] D114713: [AArch64][SVE][NEON] Add NEON-SVE-Bridge intrinsics

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added inline comments.



Comment at: 
clang/test/CodeGen/aarch64_neon_sve_bridge_intrinsics/acle_neon_sve_bridge_dup_neonq.c:8
+
+#define SVE_ACLE_FUNC(A1,A2,A3,A4) A1##A2##A3##A4
+

Do you also want to test the overloaded forms here via SVE_OVERLOADED_FORMS?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114713

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

[PATCH] D113489: [AArch64][SVE] Instcombine SVE LD1/ST1 to stock LLVM IR

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

My current thinking is that the route forward is to update the tests without 
update test checks, which affects only the st1{b,h,w} files, and possibly to 
remove the false header implying these files were autogenerated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113489

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

[PATCH] D113489: [AArch64][SVE] Instcombine SVE LD1/ST1 to stock LLVM IR

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

It looks like some auto-generation lines were added in 
91eda9c30f33da6ec6da70b59a5f5da6c6397039 
, but the 
tests using CHECK-DAG were not actually auto-generated. It seems we could 
either cleanly auto-generate them, or we should copy the existing CHECK-DAG 
patterns. It seems that what is currently committed is a little wrong, if only 
by having a 'these tests are auto-generated' header even though they are not 
auto-generated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113489

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

[PATCH] D113489: [AArch64][SVE] Instcombine SVE LD1/ST1 to stock LLVM IR

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added inline comments.



Comment at: clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_st1w.c:235
+// CHECK-NEXT:ret void
+//
 void test_svst1w_scatter_u64base_index_u64(svbool_t pg, svuint64_t bases, 
int64_t index, svuint64_t data)

Unfortunately it looks like these were not autogenerated, they used to use 
CHECK-DAG. This results in more changes than we expect. My thinking is that 
these tests are intended to be autogenerated, so we should make a clean patch 
which autogenerates the tests and then reapply your work on top of that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113489

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

[PATCH] D113776: [Clang][SVE] Properly enable/disable dependant SVE target features based upon +(no)sve.* options

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added inline comments.



Comment at: llvm/unittests/Support/TargetParserTest.cpp:908
+ AArch64::AEK_I8MM | AArch64::AEK_SVE |
+ AArch64::AEK_SVE2 | AArch64::AEK_SVE2BITPERM |
  AArch64::AEK_PAUTH | AArch64::AEK_MTE |

Has AEK_SVE2 been doubled up in this constant?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113776

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

[PATCH] D111790: [AArch64][Driver][SVE] Allow -msve-vector-bits=+ syntax to mean no maximum vscale

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

(Accept, but please run clang format)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111790

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

[PATCH] D102623: [CodeGen][AArch64][SVE] Canonicalize intrinsic rdffr{ => _z}

[Peter Waller 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 rG2d574a110440: [CodeGen][AArch64][SVE] Canonicalize intrinsic 
rdffr{ = _z} (authored by peterwaller-arm).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102623

Files:
  clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_rdffr.c
  llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
  llvm/test/CodeGen/AArch64/sve-intrinsics-ffr-manipulation.ll
  llvm/test/Transforms/InstCombine/AArch64/sve-intrinsics-rdffr-predication.ll

Index: llvm/test/Transforms/InstCombine/AArch64/sve-intrinsics-rdffr-predication.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstCombine/AArch64/sve-intrinsics-rdffr-predication.ll
@@ -0,0 +1,19 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -instcombine < %s | FileCheck %s
+
+target triple = "aarch64-unknown-linux-gnu"
+
+; Test that rdffr is substituted with predicated form which enables ptest optimization later.
+define  @predicate_rdffr() #0 {
+; CHECK-LABEL: @predicate_rdffr(
+; CHECK-NEXT:[[TMP1:%.*]] = call  @llvm.aarch64.sve.ptrue.nxv16i1(i32 31)
+; CHECK-NEXT:[[OUT:%.*]] = call  @llvm.aarch64.sve.rdffr.z( [[TMP1]])
+; CHECK-NEXT:ret  [[OUT]]
+;
+  %out = call  @llvm.aarch64.sve.rdffr()
+  ret  %out
+}
+
+declare  @llvm.aarch64.sve.rdffr()
+
+attributes #0 = { "target-features"="+sve" }
Index: llvm/test/CodeGen/AArch64/sve-intrinsics-ffr-manipulation.ll
===
--- llvm/test/CodeGen/AArch64/sve-intrinsics-ffr-manipulation.ll
+++ llvm/test/CodeGen/AArch64/sve-intrinsics-ffr-manipulation.ll
@@ -1,33 +1,51 @@
-; RUN: llc -mtriple=aarch64-linux-gnu -mattr=+sve < %s | FileCheck %s
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s | FileCheck %s
+
+target triple = "aarch64-unknown-linux-gnu"
 
 ;
 ; RDFFR
 ;
 
-define  @rdffr() {
+define  @rdffr() #0 {
 ; CHECK-LABEL: rdffr:
-; CHECK: rdffr p0.b
-; CHECK-NEXT: ret
+; CHECK:   // %bb.0:
+; CHECK-NEXT:rdffr p0.b
+; CHECK-NEXT:ret
   %out = call  @llvm.aarch64.sve.rdffr()
   ret  %out
 }
 
-define  @rdffr_z( %pg) {
+define  @rdffr_z( %pg) #0 {
 ; CHECK-LABEL: rdffr_z:
-; CHECK: rdffr p0.b, p0/z
-; CHECK-NEXT: ret
+; CHECK:   // %bb.0:
+; CHECK-NEXT:rdffr p0.b, p0/z
+; CHECK-NEXT:ret
   %out = call  @llvm.aarch64.sve.rdffr.z( %pg)
   ret  %out
 }
 
+; Test that rdffr.z followed by ptest optimizes to flags-setting rdffrs.
+define i1 @rdffr_z_ptest( %pg) #0 {
+; CHECK-LABEL: rdffr_z_ptest:
+; CHECK:   // %bb.0:
+; CHECK-NEXT:rdffrs p0.b, p0/z
+; CHECK-NEXT:cset w0, ne
+; CHECK-NEXT:ret
+  %rdffr = call  @llvm.aarch64.sve.rdffr.z( %pg)
+  %out = call i1 @llvm.aarch64.sve.ptest.any.nxv16i1( %pg,  %rdffr)
+  ret i1 %out
+}
+
 ;
 ; SETFFR
 ;
 
-define void @set_ffr() {
+define void @set_ffr() #0 {
 ; CHECK-LABEL: set_ffr:
-; CHECK: setffr
-; CHECK-NEXT: ret
+; CHECK:   // %bb.0:
+; CHECK-NEXT:setffr
+; CHECK-NEXT:ret
   call void @llvm.aarch64.sve.setffr()
   ret void
 }
@@ -36,10 +54,11 @@
 ; WRFFR
 ;
 
-define void @wrffr( %a) {
+define void @wrffr( %a) #0 {
 ; CHECK-LABEL: wrffr:
-; CHECK: wrffr p0.b
-; CHECK-NEXT: ret
+; CHECK:   // %bb.0:
+; CHECK-NEXT:wrffr p0.b
+; CHECK-NEXT:ret
   call void @llvm.aarch64.sve.wrffr( %a)
   ret void
 }
@@ -48,3 +67,7 @@
 declare  @llvm.aarch64.sve.rdffr.z()
 declare void @llvm.aarch64.sve.setffr()
 declare void @llvm.aarch64.sve.wrffr()
+
+declare i1 @llvm.aarch64.sve.ptest.any.nxv16i1(, )
+
+attributes #0 = { "target-features"="+sve" }
Index: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
===
--- llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
+++ llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
@@ -470,6 +470,23 @@
   return IC.replaceInstUsesWith(II, Extract);
 }
 
+static Optional instCombineRDFFR(InstCombiner ,
+IntrinsicInst ) {
+  LLVMContext  = II.getContext();
+  IRBuilder<> Builder(Ctx);
+  Builder.SetInsertPoint();
+  // Replace rdffr with predicated rdffr.z intrinsic, so that optimizePTestInstr
+  // can work with RDFFR_PP for ptest elimination.
+  auto *AllPat =
+  ConstantInt::get(Type::getInt32Ty(Ctx), AArch64SVEPredPattern::all);
+  auto *PTrue = Builder.CreateIntrinsic(Intrinsic::aarch64_sve_ptrue,
+{II.getType()}, {AllPat});
+  auto *RDFFR =
+  Builder.CreateIntrinsic(Intrinsic::aarch64_sve_rdffr_z, {}, {PTrue});
+  RDFFR->takeName();
+  return IC.replaceInstUsesWith(II, RDFFR);
+}
+
 Optional
 AArch64TTIImpl::instCombineIntrinsic(InstCombiner ,
  

[PATCH] D102623: [CodeGen][AArch64][SVE] Canonicalize intrinsic rdffr{ => _z}

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm updated this revision to Diff 346423.
peterwaller-arm added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

- Move test per review comment.
- Update ACLE test.
- Rebase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102623

Files:
  clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_rdffr.c
  llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
  llvm/test/CodeGen/AArch64/sve-intrinsics-ffr-manipulation.ll
  llvm/test/Transforms/InstCombine/AArch64/sve-intrinsics-rdffr-predication.ll

Index: llvm/test/Transforms/InstCombine/AArch64/sve-intrinsics-rdffr-predication.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstCombine/AArch64/sve-intrinsics-rdffr-predication.ll
@@ -0,0 +1,19 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -instcombine < %s | FileCheck %s
+
+target triple = "aarch64-unknown-linux-gnu"
+
+; Test that rdffr is substituted with predicated form which enables ptest optimization later.
+define  @predicate_rdffr() #0 {
+; CHECK-LABEL: @predicate_rdffr(
+; CHECK-NEXT:[[TMP1:%.*]] = call  @llvm.aarch64.sve.ptrue.nxv16i1(i32 31)
+; CHECK-NEXT:[[OUT:%.*]] = call  @llvm.aarch64.sve.rdffr.z( [[TMP1]])
+; CHECK-NEXT:ret  [[OUT]]
+;
+  %out = call  @llvm.aarch64.sve.rdffr()
+  ret  %out
+}
+
+declare  @llvm.aarch64.sve.rdffr()
+
+attributes #0 = { "target-features"="+sve" }
Index: llvm/test/CodeGen/AArch64/sve-intrinsics-ffr-manipulation.ll
===
--- llvm/test/CodeGen/AArch64/sve-intrinsics-ffr-manipulation.ll
+++ llvm/test/CodeGen/AArch64/sve-intrinsics-ffr-manipulation.ll
@@ -1,33 +1,51 @@
-; RUN: llc -mtriple=aarch64-linux-gnu -mattr=+sve < %s | FileCheck %s
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s | FileCheck %s
+
+target triple = "aarch64-unknown-linux-gnu"
 
 ;
 ; RDFFR
 ;
 
-define  @rdffr() {
+define  @rdffr() #0 {
 ; CHECK-LABEL: rdffr:
-; CHECK: rdffr p0.b
-; CHECK-NEXT: ret
+; CHECK:   // %bb.0:
+; CHECK-NEXT:rdffr p0.b
+; CHECK-NEXT:ret
   %out = call  @llvm.aarch64.sve.rdffr()
   ret  %out
 }
 
-define  @rdffr_z( %pg) {
+define  @rdffr_z( %pg) #0 {
 ; CHECK-LABEL: rdffr_z:
-; CHECK: rdffr p0.b, p0/z
-; CHECK-NEXT: ret
+; CHECK:   // %bb.0:
+; CHECK-NEXT:rdffr p0.b, p0/z
+; CHECK-NEXT:ret
   %out = call  @llvm.aarch64.sve.rdffr.z( %pg)
   ret  %out
 }
 
+; Test that rdffr.z followed by ptest optimizes to flags-setting rdffrs.
+define i1 @rdffr_z_ptest( %pg) #0 {
+; CHECK-LABEL: rdffr_z_ptest:
+; CHECK:   // %bb.0:
+; CHECK-NEXT:rdffrs p0.b, p0/z
+; CHECK-NEXT:cset w0, ne
+; CHECK-NEXT:ret
+  %rdffr = call  @llvm.aarch64.sve.rdffr.z( %pg)
+  %out = call i1 @llvm.aarch64.sve.ptest.any.nxv16i1( %pg,  %rdffr)
+  ret i1 %out
+}
+
 ;
 ; SETFFR
 ;
 
-define void @set_ffr() {
+define void @set_ffr() #0 {
 ; CHECK-LABEL: set_ffr:
-; CHECK: setffr
-; CHECK-NEXT: ret
+; CHECK:   // %bb.0:
+; CHECK-NEXT:setffr
+; CHECK-NEXT:ret
   call void @llvm.aarch64.sve.setffr()
   ret void
 }
@@ -36,10 +54,11 @@
 ; WRFFR
 ;
 
-define void @wrffr( %a) {
+define void @wrffr( %a) #0 {
 ; CHECK-LABEL: wrffr:
-; CHECK: wrffr p0.b
-; CHECK-NEXT: ret
+; CHECK:   // %bb.0:
+; CHECK-NEXT:wrffr p0.b
+; CHECK-NEXT:ret
   call void @llvm.aarch64.sve.wrffr( %a)
   ret void
 }
@@ -48,3 +67,7 @@
 declare  @llvm.aarch64.sve.rdffr.z()
 declare void @llvm.aarch64.sve.setffr()
 declare void @llvm.aarch64.sve.wrffr()
+
+declare i1 @llvm.aarch64.sve.ptest.any.nxv16i1(, )
+
+attributes #0 = { "target-features"="+sve" }
Index: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
===
--- llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
+++ llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
@@ -470,6 +470,23 @@
   return IC.replaceInstUsesWith(II, Extract);
 }
 
+static Optional instCombineRDFFR(InstCombiner ,
+IntrinsicInst ) {
+  LLVMContext  = II.getContext();
+  IRBuilder<> Builder(Ctx);
+  Builder.SetInsertPoint();
+  // Replace rdffr with predicated rdffr.z intrinsic, so that optimizePTestInstr
+  // can work with RDFFR_PP for ptest elimination.
+  auto *AllPat =
+  ConstantInt::get(Type::getInt32Ty(Ctx), AArch64SVEPredPattern::all);
+  auto *PTrue = Builder.CreateIntrinsic(Intrinsic::aarch64_sve_ptrue,
+{II.getType()}, {AllPat});
+  auto *RDFFR =
+  Builder.CreateIntrinsic(Intrinsic::aarch64_sve_rdffr_z, {}, {PTrue});
+  RDFFR->takeName();
+  return IC.replaceInstUsesWith(II, RDFFR);
+}
+
 Optional
 AArch64TTIImpl::instCombineIntrinsic(InstCombiner ,
  IntrinsicInst ) const {
@@ -481,6 

[PATCH] D98030: [IR] Add vscale_range IR function attribute

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added inline comments.



Comment at: llvm/lib/IR/Attributes.cpp:570
+Result += utostr(MinValue);
+Result += ',';
+Result += utostr(MaxValue);

Nit: The only other precedent I can see for this is `allocsize`. Grepping the 
code I found this is always written in tests as `allocsize(x, y)`, however, it 
prints as `allocsize(x,y)` (no space) when done with `-emit-llvm`, regardless 
of how it was formatted as input. I figure that is an oversight and this should 
have the space.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98030

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

[PATCH] D95435: [clang][aarch64][WOA64][docs] Release note for longjmp crash with /guard:cf

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm abandoned this revision.
peterwaller-arm added a comment.

Release note upstream on release/12.x branch in 
rGbc2dad1671598a87423c61c355d03db49ce76907 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95435

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

[PATCH] D95435: [clang][aarch64][WOA64][docs] Release note for longjmp crash with /guard:cf

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm created this revision.
peterwaller-arm added a reviewer: richard.townsend.arm.
Herald added subscribers: danielkiss, kristof.beyls.
peterwaller-arm requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Add a release note workaround for PR47463.

Bug: https://bugs.llvm.org/show_bug.cgi?id=47463


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D95435

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -146,6 +146,11 @@
 Windows Support
 ---
 
+- Windows on Arm64: programs using the C standard library's setjmp and longjmp
+  functions may crash with a "Security check failure or stack buffer overrun"
+  exception. To workaround (with reduced security), compile with
+  /guard:cf,nolongjmp.
+
 C Language Changes in Clang
 ---
 


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -146,6 +146,11 @@
 Windows Support
 ---
 
+- Windows on Arm64: programs using the C standard library's setjmp and longjmp
+  functions may crash with a "Security check failure or stack buffer overrun"
+  exception. To workaround (with reduced security), compile with
+  /guard:cf,nolongjmp.
+
 C Language Changes in Clang
 ---
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92751: [clang][aarch64] Precondition isHomogeneousAggregate on isCXX14Aggregate

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm accepted this revision.
peterwaller-arm added inline comments.



Comment at: llvm/test/CodeGen/AArch64/arm64-windows-calls.ll:117
+  ret %struct.Pod %x1
+; CHECK: ldp d0, d1, [x0]
+}

rnk wrote:
> peterwaller-arm wrote:
> > rnk wrote:
> > > Please use CHECK-LABEL directives to ensure that these assembly checks 
> > > are from the function you intend to match.
> > @DavidTruby I think the intent here is:
> > 
> > CHECK-LABEL: < something matching the first line of the function definition 
> > >
> > CHECK: 
> > 
> > CHECK-LABEL: 
> > 
> > Such that if by some fluke some other function in the interim produces 
> > , it won't be allowed to match something produced in 
> > the subsequent function. This also renders the code safe against this 
> > problem if functions are added or moved around. I suggest having a 
> > CHECK-LABEL on each function heading, and the code within those functions 
> > should be CHECK/CHECK-NEXT as appropriate.
> Yep. I think you could still go further by adding a trailing colon. FileCheck 
> matches for substrings of a line, so as written, this could match "call 
> copy_pod", which wouldn't be what you want. The risk of that in this test is 
> low, but it's a good best practice.
To echo @rnk, ideally your CHECK-LABEL statements should match the function 
header, and typically they're placed immediately above the function header, 
since that's logically what they're matching. The existence of the CHECK-LABEL 
primitive on every function header constrains the checks to only match within 
their intended function between the 'bracket' of two CHECK-LABEL statements.

```

; CHECK-LABEL: define {{.*}}thing
void thing() {
; CHECK: some codegen in thing
// ... do codegen here.
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92751

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

[PATCH] D92751: [clang][aarch64] Precondition isHomogeneousAggregate on isCXX14Aggregate

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

Suggesting a few small tweaks. Otherwise LGTM when @rnk is happy.




Comment at: clang/test/CodeGenCXX/homogeneous-aggregates.cpp:129
+struct Empty {};
+// A class with a base is returned using the sret calling convetion by MSVC.
+struct HasEmptyBase : public Empty {





Comment at: clang/test/CodeGenCXX/homogeneous-aggregates.cpp:147
+  *pod = copy(pod);
+  // WOA64-LABEL: %{{.*}} = call %"struct.pr47611::Pod" 
@"?copy@pr47611@@YA?AUPod@1@PEAU21@@Z"(%"struct.pr47611::Pod"* %{{.*}})
+}

Same comment (from below) about -LABEL for each of these functions. This should 
be a 'check'-alike and each function header should have a CHECK-LABEL.



Comment at: llvm/test/CodeGen/AArch64/arm64-windows-calls.ll:117
+  ret %struct.Pod %x1
+; CHECK: ldp d0, d1, [x0]
+}

rnk wrote:
> Please use CHECK-LABEL directives to ensure that these assembly checks are 
> from the function you intend to match.
@DavidTruby I think the intent here is:

CHECK-LABEL: < something matching the first line of the function definition >
CHECK: 

CHECK-LABEL: 

Such that if by some fluke some other function in the interim produces , it won't be allowed to match something produced in the 
subsequent function. This also renders the code safe against this problem if 
functions are added or moved around. I suggest having a CHECK-LABEL on each 
function heading, and the code within those functions should be 
CHECK/CHECK-NEXT as appropriate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92751

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

[PATCH] D91806: [InstCombine] Update valueCoversEntireFragment to use TypeSize

[Peter Waller 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 rGdfd3384feeca: [InstCombine] Update valueCoversEntireFragment 
to use TypeSize (authored by fpetrogalli, committed by peterwaller-arm).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91806

Files:
  llvm/lib/Transforms/Utils/Local.cpp
  llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll


Index: llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll
@@ -0,0 +1,36 @@
+; RUN: opt -instcombine -S < %s 2>%t | FileCheck %s
+; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t
+
+; This test is defending against a TypeSize message raised in the method
+; `valueCoversEntireFragment` in Local.cpp because of an implicit cast from
+; `TypeSize` to `uint64_t`. This particular TypeSize message only occurred when
+; debug info was available.
+
+; If this check fails please read
+; clang/test/CodeGen/aarch64-sve-intrinsics/README for instructions on
+; how to resolve it.
+; This test must not produce any warnings. Prior to this test being introduced,
+; it produced a warning containing the text "TypeSize is not scalable".
+; WARN-NOT: warning:
+
+; CHECK-LABEL: @debug_local_scalable(
+define  @debug_local_scalable( 
%tostore) {
+  %vx = alloca , align 16
+  call void @llvm.dbg.declare(metadata * %vx, metadata 
!3, metadata !DIExpression()), !dbg !5
+  store  %tostore, * %vx, align 16
+  %ret = call  @f(* %vx)
+  ret  %ret
+}
+
+declare  @f(*)
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+!llvm.module.flags = !{!2}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1)
+!1 = !DIFile(filename: "/tmp/test.c", directory: "/tmp/")
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = !DILocalVariable(scope: !4)
+!4 = distinct !DISubprogram(unit: !0)
+!5 = !DILocation(scope: !4)
Index: llvm/lib/Transforms/Utils/Local.cpp
===
--- llvm/lib/Transforms/Utils/Local.cpp
+++ llvm/lib/Transforms/Utils/Local.cpp
@@ -1340,16 +1340,22 @@
 /// least n bits.
 static bool valueCoversEntireFragment(Type *ValTy, DbgVariableIntrinsic *DII) {
   const DataLayout  = DII->getModule()->getDataLayout();
-  uint64_t ValueSize = DL.getTypeAllocSizeInBits(ValTy);
-  if (auto FragmentSize = DII->getFragmentSizeInBits())
-return ValueSize >= *FragmentSize;
+  TypeSize ValueSize = DL.getTypeAllocSizeInBits(ValTy);
+  if (Optional FragmentSize = DII->getFragmentSizeInBits()) {
+assert(!ValueSize.isScalable() &&
+   "Fragments don't work on scalable types.");
+return ValueSize.getFixedSize() >= *FragmentSize;
+  }
   // We can't always calculate the size of the DI variable (e.g. if it is a
   // VLA). Try to use the size of the alloca that the dbg intrinsic describes
   // intead.
   if (DII->isAddressOfVariable())
 if (auto *AI = dyn_cast_or_null(DII->getVariableLocation()))
-  if (auto FragmentSize = AI->getAllocationSizeInBits(DL))
-return ValueSize >= *FragmentSize;
+  if (Optional FragmentSize = AI->getAllocationSizeInBits(DL)) {
+assert(ValueSize.isScalable() == FragmentSize->isScalable() &&
+   "Both sizes should agree on the scalable flag.");
+return TypeSize::isKnownGE(ValueSize, *FragmentSize);
+  }
   // Could not determine size of variable. Conservatively return false.
   return false;
 }


Index: llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll
@@ -0,0 +1,36 @@
+; RUN: opt -instcombine -S < %s 2>%t | FileCheck %s
+; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t
+
+; This test is defending against a TypeSize message raised in the method
+; `valueCoversEntireFragment` in Local.cpp because of an implicit cast from
+; `TypeSize` to `uint64_t`. This particular TypeSize message only occurred when
+; debug info was available.
+
+; If this check fails please read
+; clang/test/CodeGen/aarch64-sve-intrinsics/README for instructions on
+; how to resolve it.
+; This test must not produce any warnings. Prior to this test being introduced,
+; it produced a warning containing the text "TypeSize is not scalable".
+; WARN-NOT: warning:
+
+; CHECK-LABEL: @debug_local_scalable(
+define  @debug_local_scalable( %tostore) {
+  %vx = alloca , align 16
+  call void @llvm.dbg.declare(metadata * %vx, metadata !3, metadata !DIExpression()), !dbg !5
+  store  %tostore, * %vx, align 16
+  %ret = call  @f(* %vx)
+  ret  %ret
+}
+
+declare  @f(*)
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+!llvm.module.flags = 

[PATCH] D91806: [InstCombine] Update valueCoversEntireFragment to use TypeSize

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm marked an inline comment as done.
peterwaller-arm added inline comments.



Comment at: llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll:13
+
+; ERR-NOT: TypeSize is not scalable
+

peterwaller-arm wrote:
> sdesmalen wrote:
> > After your changes, what line causes this warning to be emitted? Does this 
> > test also pass with asserts enabled?
> It's a -NOT check, so it's not being emitted. Yes, the test passes even with 
> asserts enabled.
> 
> I assume the question is motivated by putting the check next to the line 
> which causes the warning(?).
Discussed out of band. Sander's interest was to disallow any warning output, as 
with other SVE ASM-NOT tests in the clang directory.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91806

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

[PATCH] D91806: [InstCombine] Update valueCoversEntireFragment to use TypeSize

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm updated this revision to Diff 314629.
peterwaller-arm added a comment.

Update for review comment.

- Generalize WARN-NOT to disallow any warnings, not just those with specific 
text.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91806

Files:
  llvm/lib/Transforms/Utils/Local.cpp
  llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll


Index: llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll
@@ -0,0 +1,36 @@
+; RUN: opt -instcombine -S < %s 2>%t | FileCheck %s
+; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t
+
+; This test is defending against a TypeSize message raised in the method
+; `valueCoversEntireFragment` in Local.cpp because of an implicit cast from
+; `TypeSize` to `uint64_t`. This particular TypeSize message only occurred when
+; debug info was available.
+
+; If this check fails please read
+; clang/test/CodeGen/aarch64-sve-intrinsics/README for instructions on
+; how to resolve it.
+; This test must not produce any warnings. Prior to this test being introduced,
+; it produced a warning containing the text "TypeSize is not scalable".
+; WARN-NOT: warning:
+
+; CHECK-LABEL: @debug_local_scalable(
+define  @debug_local_scalable( 
%tostore) {
+  %vx = alloca , align 16
+  call void @llvm.dbg.declare(metadata * %vx, metadata 
!3, metadata !DIExpression()), !dbg !5
+  store  %tostore, * %vx, align 16
+  %ret = call  @f(* %vx)
+  ret  %ret
+}
+
+declare  @f(*)
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+!llvm.module.flags = !{!2}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1)
+!1 = !DIFile(filename: "/tmp/test.c", directory: "/tmp/")
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = !DILocalVariable(scope: !4)
+!4 = distinct !DISubprogram(unit: !0)
+!5 = !DILocation(scope: !4)
Index: llvm/lib/Transforms/Utils/Local.cpp
===
--- llvm/lib/Transforms/Utils/Local.cpp
+++ llvm/lib/Transforms/Utils/Local.cpp
@@ -1340,16 +1340,22 @@
 /// least n bits.
 static bool valueCoversEntireFragment(Type *ValTy, DbgVariableIntrinsic *DII) {
   const DataLayout  = DII->getModule()->getDataLayout();
-  uint64_t ValueSize = DL.getTypeAllocSizeInBits(ValTy);
-  if (auto FragmentSize = DII->getFragmentSizeInBits())
-return ValueSize >= *FragmentSize;
+  TypeSize ValueSize = DL.getTypeAllocSizeInBits(ValTy);
+  if (Optional FragmentSize = DII->getFragmentSizeInBits()) {
+assert(!ValueSize.isScalable() &&
+   "Fragments don't work on scalable types.");
+return ValueSize.getFixedSize() >= *FragmentSize;
+  }
   // We can't always calculate the size of the DI variable (e.g. if it is a
   // VLA). Try to use the size of the alloca that the dbg intrinsic describes
   // intead.
   if (DII->isAddressOfVariable())
 if (auto *AI = dyn_cast_or_null(DII->getVariableLocation()))
-  if (auto FragmentSize = AI->getAllocationSizeInBits(DL))
-return ValueSize >= *FragmentSize;
+  if (Optional FragmentSize = AI->getAllocationSizeInBits(DL)) {
+assert(ValueSize.isScalable() == FragmentSize->isScalable() &&
+   "Both sizes should agree on the scalable flag.");
+return TypeSize::isKnownGE(ValueSize, *FragmentSize);
+  }
   // Could not determine size of variable. Conservatively return false.
   return false;
 }


Index: llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll
@@ -0,0 +1,36 @@
+; RUN: opt -instcombine -S < %s 2>%t | FileCheck %s
+; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t
+
+; This test is defending against a TypeSize message raised in the method
+; `valueCoversEntireFragment` in Local.cpp because of an implicit cast from
+; `TypeSize` to `uint64_t`. This particular TypeSize message only occurred when
+; debug info was available.
+
+; If this check fails please read
+; clang/test/CodeGen/aarch64-sve-intrinsics/README for instructions on
+; how to resolve it.
+; This test must not produce any warnings. Prior to this test being introduced,
+; it produced a warning containing the text "TypeSize is not scalable".
+; WARN-NOT: warning:
+
+; CHECK-LABEL: @debug_local_scalable(
+define  @debug_local_scalable( %tostore) {
+  %vx = alloca , align 16
+  call void @llvm.dbg.declare(metadata * %vx, metadata !3, metadata !DIExpression()), !dbg !5
+  store  %tostore, * %vx, align 16
+  %ret = call  @f(* %vx)
+  ret  %ret
+}
+
+declare  @f(*)
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+!llvm.module.flags = !{!2}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1)
+!1 = 

[PATCH] D91806: [InstCombine] Update valueCoversEntireFragment to use TypeSize

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added inline comments.



Comment at: llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll:13
+
+; ERR-NOT: TypeSize is not scalable
+

sdesmalen wrote:
> After your changes, what line causes this warning to be emitted? Does this 
> test also pass with asserts enabled?
It's a -NOT check, so it's not being emitted. Yes, the test passes even with 
asserts enabled.

I assume the question is motivated by putting the check next to the line which 
causes the warning(?).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91806

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

[PATCH] D91806: [InstCombine] Update valueCoversEntireFragment to use TypeSize

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm updated this revision to Diff 311614.
peterwaller-arm retitled this revision from "[SVE] Change 
getTypeAllocationSizeInBits to return TypeSize" to "[InstCombine] Update 
valueCoversEntireFragment to use TypeSize".
peterwaller-arm edited the summary of this revision.
peterwaller-arm added a comment.

Attempting to update commit message with `arc diff --verbatim`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91806

Files:
  llvm/lib/Transforms/Utils/Local.cpp
  llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll


Index: llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll
@@ -0,0 +1,35 @@
+; RUN: opt -instcombine -S < %s 2>%t | FileCheck %s
+; RUN: FileCheck --check-prefix=ERR --allow-empty %s <%t
+
+; This test is defending against a TypeSize message raised in the method
+; `valueCoversEntireFragment` in Local.cpp because of an implicit cast from
+; `TypeSize` to `uint64_t`. This particular TypeSize message only occurred when
+; debug info was available.
+
+; If this check fails please read
+; clang/test/CodeGen/aarch64-sve-intrinsics/README for instructions on
+; how to resolve it.
+
+; ERR-NOT: TypeSize is not scalable
+
+; CHECK-LABEL: @debug_local_scalable(
+define  @debug_local_scalable( 
%tostore) {
+  %vx = alloca , align 16
+  call void @llvm.dbg.declare(metadata * %vx, metadata 
!3, metadata !DIExpression()), !dbg !5
+  store  %tostore, * %vx, align 16
+  %ret = call  @f(* %vx)
+  ret  %ret
+}
+
+declare  @f(*)
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+!llvm.module.flags = !{!2}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1)
+!1 = !DIFile(filename: "/tmp/test.c", directory: "/tmp/")
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = !DILocalVariable(scope: !4)
+!4 = distinct !DISubprogram(unit: !0)
+!5 = !DILocation(scope: !4)
Index: llvm/lib/Transforms/Utils/Local.cpp
===
--- llvm/lib/Transforms/Utils/Local.cpp
+++ llvm/lib/Transforms/Utils/Local.cpp
@@ -1368,16 +1368,22 @@
 /// least n bits.
 static bool valueCoversEntireFragment(Type *ValTy, DbgVariableIntrinsic *DII) {
   const DataLayout  = DII->getModule()->getDataLayout();
-  uint64_t ValueSize = DL.getTypeAllocSizeInBits(ValTy);
-  if (auto FragmentSize = DII->getFragmentSizeInBits())
-return ValueSize >= *FragmentSize;
+  TypeSize ValueSize = DL.getTypeAllocSizeInBits(ValTy);
+  if (Optional FragmentSize = DII->getFragmentSizeInBits()) {
+assert(!ValueSize.isScalable() &&
+   "Fragments don't work on scalable types.");
+return ValueSize.getFixedSize() >= *FragmentSize;
+  }
   // We can't always calculate the size of the DI variable (e.g. if it is a
   // VLA). Try to use the size of the alloca that the dbg intrinsic describes
   // intead.
   if (DII->isAddressOfVariable())
 if (auto *AI = dyn_cast_or_null(DII->getVariableLocation()))
-  if (auto FragmentSize = AI->getAllocationSizeInBits(DL))
-return ValueSize >= *FragmentSize;
+  if (Optional FragmentSize = AI->getAllocationSizeInBits(DL)) {
+assert(ValueSize.isScalable() == FragmentSize->isScalable() &&
+   "Both sizes should agree on the scalable flag.");
+return TypeSize::isKnownGE(ValueSize, *FragmentSize);
+  }
   // Could not determine size of variable. Conservatively return false.
   return false;
 }


Index: llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll
@@ -0,0 +1,35 @@
+; RUN: opt -instcombine -S < %s 2>%t | FileCheck %s
+; RUN: FileCheck --check-prefix=ERR --allow-empty %s <%t
+
+; This test is defending against a TypeSize message raised in the method
+; `valueCoversEntireFragment` in Local.cpp because of an implicit cast from
+; `TypeSize` to `uint64_t`. This particular TypeSize message only occurred when
+; debug info was available.
+
+; If this check fails please read
+; clang/test/CodeGen/aarch64-sve-intrinsics/README for instructions on
+; how to resolve it.
+
+; ERR-NOT: TypeSize is not scalable
+
+; CHECK-LABEL: @debug_local_scalable(
+define  @debug_local_scalable( %tostore) {
+  %vx = alloca , align 16
+  call void @llvm.dbg.declare(metadata * %vx, metadata !3, metadata !DIExpression()), !dbg !5
+  store  %tostore, * %vx, align 16
+  %ret = call  @f(* %vx)
+  ret  %ret
+}
+
+declare  @f(*)
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+!llvm.module.flags = !{!2}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1)
+!1 = !DIFile(filename: "/tmp/test.c", directory: "/tmp/")
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = 

[PATCH] D91806: [SVE] Change getTypeAllocationSizeInBits to return TypeSize

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm updated this revision to Diff 311612.
peterwaller-arm added a comment.

I've taken over the differential.

Simplify things a bit more, remove an untested remnant created from before the 
patch split.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91806

Files:
  llvm/lib/Transforms/Utils/Local.cpp
  llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll


Index: llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll
@@ -0,0 +1,35 @@
+; RUN: opt -instcombine -S < %s 2>%t | FileCheck %s
+; RUN: FileCheck --check-prefix=ERR --allow-empty %s <%t
+
+; This test is defending against a TypeSize message raised in the method
+; `valueCoversEntireFragment` in Local.cpp because of an implicit cast from
+; `TypeSize` to `uint64_t`. This particular TypeSize message only occurred when
+; debug info was available.
+
+; If this check fails please read
+; clang/test/CodeGen/aarch64-sve-intrinsics/README for instructions on
+; how to resolve it.
+
+; ERR-NOT: TypeSize is not scalable
+
+; CHECK-LABEL: @debug_local_scalable(
+define  @debug_local_scalable( 
%tostore) {
+  %vx = alloca , align 16
+  call void @llvm.dbg.declare(metadata * %vx, metadata 
!3, metadata !DIExpression()), !dbg !5
+  store  %tostore, * %vx, align 16
+  %ret = call  @f(* %vx)
+  ret  %ret
+}
+
+declare  @f(*)
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+!llvm.module.flags = !{!2}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1)
+!1 = !DIFile(filename: "/tmp/test.c", directory: "/tmp/")
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = !DILocalVariable(scope: !4)
+!4 = distinct !DISubprogram(unit: !0)
+!5 = !DILocation(scope: !4)
Index: llvm/lib/Transforms/Utils/Local.cpp
===
--- llvm/lib/Transforms/Utils/Local.cpp
+++ llvm/lib/Transforms/Utils/Local.cpp
@@ -1368,16 +1368,22 @@
 /// least n bits.
 static bool valueCoversEntireFragment(Type *ValTy, DbgVariableIntrinsic *DII) {
   const DataLayout  = DII->getModule()->getDataLayout();
-  uint64_t ValueSize = DL.getTypeAllocSizeInBits(ValTy);
-  if (auto FragmentSize = DII->getFragmentSizeInBits())
-return ValueSize >= *FragmentSize;
+  TypeSize ValueSize = DL.getTypeAllocSizeInBits(ValTy);
+  if (Optional FragmentSize = DII->getFragmentSizeInBits()) {
+assert(!ValueSize.isScalable() &&
+   "Fragments don't work on scalable types.");
+return ValueSize.getFixedSize() >= *FragmentSize;
+  }
   // We can't always calculate the size of the DI variable (e.g. if it is a
   // VLA). Try to use the size of the alloca that the dbg intrinsic describes
   // intead.
   if (DII->isAddressOfVariable())
 if (auto *AI = dyn_cast_or_null(DII->getVariableLocation()))
-  if (auto FragmentSize = AI->getAllocationSizeInBits(DL))
-return ValueSize >= *FragmentSize;
+  if (Optional FragmentSize = AI->getAllocationSizeInBits(DL)) {
+assert(ValueSize.isScalable() == FragmentSize->isScalable() &&
+   "Both sizes should agree on the scalable flag.");
+return TypeSize::isKnownGE(ValueSize, *FragmentSize);
+  }
   // Could not determine size of variable. Conservatively return false.
   return false;
 }


Index: llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll
@@ -0,0 +1,35 @@
+; RUN: opt -instcombine -S < %s 2>%t | FileCheck %s
+; RUN: FileCheck --check-prefix=ERR --allow-empty %s <%t
+
+; This test is defending against a TypeSize message raised in the method
+; `valueCoversEntireFragment` in Local.cpp because of an implicit cast from
+; `TypeSize` to `uint64_t`. This particular TypeSize message only occurred when
+; debug info was available.
+
+; If this check fails please read
+; clang/test/CodeGen/aarch64-sve-intrinsics/README for instructions on
+; how to resolve it.
+
+; ERR-NOT: TypeSize is not scalable
+
+; CHECK-LABEL: @debug_local_scalable(
+define  @debug_local_scalable( %tostore) {
+  %vx = alloca , align 16
+  call void @llvm.dbg.declare(metadata * %vx, metadata !3, metadata !DIExpression()), !dbg !5
+  store  %tostore, * %vx, align 16
+  %ret = call  @f(* %vx)
+  ret  %ret
+}
+
+declare  @f(*)
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+!llvm.module.flags = !{!2}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1)
+!1 = !DIFile(filename: "/tmp/test.c", directory: "/tmp/")
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = !DILocalVariable(scope: !4)
+!4 = distinct !DISubprogram(unit: !0)
+!5 = !DILocation(scope: !4)
Index: llvm/lib/Transforms/Utils/Local.cpp

[PATCH] D92487: [AArch64][Driver][SVE] Push missing SVE feature error from driver to frontend

[Peter Waller 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 rG2315e9874c92: [AArch64][Driver][SVE] Push missing SVE 
feature error from driver to frontend (authored by peterwaller-arm).
Herald added a subscriber: NickHung.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92487

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Driver/aarch64-sve-vector-bits.c
  clang/test/Sema/arm-vector-types-support.c
  clang/test/Sema/neon-vector-types-support.c

Index: clang/test/Sema/neon-vector-types-support.c
===
--- clang/test/Sema/neon-vector-types-support.c
+++ /dev/null
@@ -1,4 +0,0 @@
-// RUN: %clang_cc1 %s -triple armv7 -fsyntax-only -verify
-
-typedef __attribute__((neon_vector_type(2))) int int32x2_t; // expected-error{{'neon_vector_type' attribute is not supported for this target}}
-typedef __attribute__((neon_polyvector_type(16))) short poly8x16_t; // expected-error{{'neon_polyvector_type' attribute is not supported for this target}}
Index: clang/test/Sema/arm-vector-types-support.c
===
--- /dev/null
+++ clang/test/Sema/arm-vector-types-support.c
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 %s -triple armv7 -fsyntax-only -verify
+
+typedef __attribute__((neon_vector_type(2))) int int32x2_t; // expected-error{{'neon_vector_type' attribute is not supported on targets missing 'neon' or 'mve'; specify an appropriate -march= or -mcpu=}}
+typedef __attribute__((neon_polyvector_type(16))) short poly8x16_t; // expected-error{{'neon_polyvector_type' attribute is not supported on targets missing 'neon' or 'mve'; specify an appropriate -march= or -mcpu=}}
+typedef __attribute__((arm_sve_vector_bits(256))) void nosveflag; // expected-error{{'arm_sve_vector_bits' attribute is not supported on targets missing 'sve'; specify an appropriate -march= or -mcpu=}}
Index: clang/test/Driver/aarch64-sve-vector-bits.c
===
--- clang/test/Driver/aarch64-sve-vector-bits.c
+++ clang/test/Driver/aarch64-sve-vector-bits.c
@@ -22,21 +22,6 @@
 // CHECK-2048: "-msve-vector-bits=2048"
 // CHECK-SCALABLE-NOT: "-msve-vector-bits=
 
-// Bail out if -msve-vector-bits is specified without SVE enabled
-// -
-// RUN: %clang -c %s -### -target aarch64-none-linux-gnu -msve-vector-bits=128 \
-// RUN:  2>&1 | FileCheck --check-prefix=CHECK-NO-SVE-ERROR %s
-// RUN: %clang -c %s -### -target aarch64-none-linux-gnu -msve-vector-bits=256 \
-// RUN:  2>&1 | FileCheck --check-prefix=CHECK-NO-SVE-ERROR %s
-// RUN: %clang -c %s -### -target aarch64-none-linux-gnu -msve-vector-bits=512 \
-// RUN:  2>&1 | FileCheck --check-prefix=CHECK-NO-SVE-ERROR %s
-// RUN: %clang -c %s -### -target aarch64-none-linux-gnu -msve-vector-bits=1024 \
-// RUN:  2>&1 | FileCheck --check-prefix=CHECK-NO-SVE-ERROR %s
-// RUN: %clang -c %s -### -target aarch64-none-linux-gnu -msve-vector-bits=2048 \
-// RUN:  2>&1 | FileCheck --check-prefix=CHECK-NO-SVE-ERROR %s
-
-// CHECK-NO-SVE-ERROR: error: '-msve-vector-bits' is not supported without SVE enabled
-
 // Error out if an unsupported value is passed to -msve-vector-bits.
 // -
 // RUN: %clang -c %s -### -target aarch64-none-linux-gnu -march=armv8-a+sve \
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -7799,7 +7799,8 @@
   // not to need a separate attribute)
   if (!S.Context.getTargetInfo().hasFeature("neon") &&
   !S.Context.getTargetInfo().hasFeature("mve")) {
-S.Diag(Attr.getLoc(), diag::err_attribute_unsupported) << Attr;
+S.Diag(Attr.getLoc(), diag::err_attribute_unsupported)
+<< Attr << "'neon' or 'mve'";
 Attr.setInvalid();
 return;
   }
@@ -7842,7 +7843,7 @@
Sema ) {
   // Target must have SVE.
   if (!S.Context.getTargetInfo().hasFeature("sve")) {
-S.Diag(Attr.getLoc(), diag::err_attribute_unsupported) << Attr;
+S.Diag(Attr.getLoc(), diag::err_attribute_unsupported) << Attr << "'sve'";
 Attr.setInvalid();
 return;
   }
Index: clang/lib/Driver/ToolChains/Arch/AArch64.cpp
===
--- clang/lib/Driver/ToolChains/Arch/AArch64.cpp
+++ clang/lib/Driver/ToolChains/Arch/AArch64.cpp
@@ -381,12 +381,6 @@
   if (V8_6Pos != std::end(Features))
 V8_6Pos = Features.insert(std::next(V8_6Pos), {"+i8mm", "+bf16"});
 
-  bool 

[PATCH] D92487: [AArch64][Driver][SVE] Push missing SVE feature error from driver to frontend

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm updated this revision to Diff 310215.
peterwaller-arm added a comment.

Update patch for clang-format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92487

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Driver/aarch64-sve-vector-bits.c
  clang/test/Sema/arm-vector-types-support.c
  clang/test/Sema/neon-vector-types-support.c

Index: clang/test/Sema/neon-vector-types-support.c
===
--- clang/test/Sema/neon-vector-types-support.c
+++ /dev/null
@@ -1,4 +0,0 @@
-// RUN: %clang_cc1 %s -triple armv7 -fsyntax-only -verify
-
-typedef __attribute__((neon_vector_type(2))) int int32x2_t; // expected-error{{'neon_vector_type' attribute is not supported for this target}}
-typedef __attribute__((neon_polyvector_type(16))) short poly8x16_t; // expected-error{{'neon_polyvector_type' attribute is not supported for this target}}
Index: clang/test/Sema/arm-vector-types-support.c
===
--- /dev/null
+++ clang/test/Sema/arm-vector-types-support.c
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 %s -triple armv7 -fsyntax-only -verify
+
+typedef __attribute__((neon_vector_type(2))) int int32x2_t; // expected-error{{'neon_vector_type' attribute is not supported on targets missing 'neon' or 'mve'; specify an appropriate -march= or -mcpu=}}
+typedef __attribute__((neon_polyvector_type(16))) short poly8x16_t; // expected-error{{'neon_polyvector_type' attribute is not supported on targets missing 'neon' or 'mve'; specify an appropriate -march= or -mcpu=}}
+typedef __attribute__((arm_sve_vector_bits(256))) void nosveflag; // expected-error{{'arm_sve_vector_bits' attribute is not supported on targets missing 'sve'; specify an appropriate -march= or -mcpu=}}
Index: clang/test/Driver/aarch64-sve-vector-bits.c
===
--- clang/test/Driver/aarch64-sve-vector-bits.c
+++ clang/test/Driver/aarch64-sve-vector-bits.c
@@ -22,21 +22,6 @@
 // CHECK-2048: "-msve-vector-bits=2048"
 // CHECK-SCALABLE-NOT: "-msve-vector-bits=
 
-// Bail out if -msve-vector-bits is specified without SVE enabled
-// -
-// RUN: %clang -c %s -### -target aarch64-none-linux-gnu -msve-vector-bits=128 \
-// RUN:  2>&1 | FileCheck --check-prefix=CHECK-NO-SVE-ERROR %s
-// RUN: %clang -c %s -### -target aarch64-none-linux-gnu -msve-vector-bits=256 \
-// RUN:  2>&1 | FileCheck --check-prefix=CHECK-NO-SVE-ERROR %s
-// RUN: %clang -c %s -### -target aarch64-none-linux-gnu -msve-vector-bits=512 \
-// RUN:  2>&1 | FileCheck --check-prefix=CHECK-NO-SVE-ERROR %s
-// RUN: %clang -c %s -### -target aarch64-none-linux-gnu -msve-vector-bits=1024 \
-// RUN:  2>&1 | FileCheck --check-prefix=CHECK-NO-SVE-ERROR %s
-// RUN: %clang -c %s -### -target aarch64-none-linux-gnu -msve-vector-bits=2048 \
-// RUN:  2>&1 | FileCheck --check-prefix=CHECK-NO-SVE-ERROR %s
-
-// CHECK-NO-SVE-ERROR: error: '-msve-vector-bits' is not supported without SVE enabled
-
 // Error out if an unsupported value is passed to -msve-vector-bits.
 // -
 // RUN: %clang -c %s -### -target aarch64-none-linux-gnu -march=armv8-a+sve \
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -7799,7 +7799,8 @@
   // not to need a separate attribute)
   if (!S.Context.getTargetInfo().hasFeature("neon") &&
   !S.Context.getTargetInfo().hasFeature("mve")) {
-S.Diag(Attr.getLoc(), diag::err_attribute_unsupported) << Attr;
+S.Diag(Attr.getLoc(), diag::err_attribute_unsupported)
+<< Attr << "'neon' or 'mve'";
 Attr.setInvalid();
 return;
   }
@@ -7842,7 +7843,7 @@
Sema ) {
   // Target must have SVE.
   if (!S.Context.getTargetInfo().hasFeature("sve")) {
-S.Diag(Attr.getLoc(), diag::err_attribute_unsupported) << Attr;
+S.Diag(Attr.getLoc(), diag::err_attribute_unsupported) << Attr << "'sve'";
 Attr.setInvalid();
 return;
   }
Index: clang/lib/Driver/ToolChains/Arch/AArch64.cpp
===
--- clang/lib/Driver/ToolChains/Arch/AArch64.cpp
+++ clang/lib/Driver/ToolChains/Arch/AArch64.cpp
@@ -381,12 +381,6 @@
   if (V8_6Pos != std::end(Features))
 V8_6Pos = Features.insert(std::next(V8_6Pos), {"+i8mm", "+bf16"});
 
-  bool HasSve = llvm::is_contained(Features, "+sve");
-  // -msve-vector-bits= flag is valid only if SVE is enabled.
-  if (Args.hasArg(options::OPT_msve_vector_bits_EQ))
-if (!HasSve)
-  

[PATCH] D92487: [AArch64][Driver][SVE] Push missing SVE feature error from driver to frontend

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm updated this revision to Diff 308989.
peterwaller-arm added a comment.

Tweak message to address review comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92487

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Driver/aarch64-sve-vector-bits.c
  clang/test/Sema/arm-vector-types-support.c
  clang/test/Sema/neon-vector-types-support.c

Index: clang/test/Sema/neon-vector-types-support.c
===
--- clang/test/Sema/neon-vector-types-support.c
+++ /dev/null
@@ -1,4 +0,0 @@
-// RUN: %clang_cc1 %s -triple armv7 -fsyntax-only -verify
-
-typedef __attribute__((neon_vector_type(2))) int int32x2_t; // expected-error{{'neon_vector_type' attribute is not supported for this target}}
-typedef __attribute__((neon_polyvector_type(16))) short poly8x16_t; // expected-error{{'neon_polyvector_type' attribute is not supported for this target}}
Index: clang/test/Sema/arm-vector-types-support.c
===
--- /dev/null
+++ clang/test/Sema/arm-vector-types-support.c
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 %s -triple armv7 -fsyntax-only -verify
+
+typedef __attribute__((neon_vector_type(2))) int int32x2_t; // expected-error{{'neon_vector_type' attribute is not supported on targets missing 'neon' or 'mve'; specify an appropriate -march= or -mcpu=}}
+typedef __attribute__((neon_polyvector_type(16))) short poly8x16_t; // expected-error{{'neon_polyvector_type' attribute is not supported on targets missing 'neon' or 'mve'; specify an appropriate -march= or -mcpu=}}
+typedef __attribute__((arm_sve_vector_bits(256))) void nosveflag; // expected-error{{'arm_sve_vector_bits' attribute is not supported on targets missing 'sve'; specify an appropriate -march= or -mcpu=}}
Index: clang/test/Driver/aarch64-sve-vector-bits.c
===
--- clang/test/Driver/aarch64-sve-vector-bits.c
+++ clang/test/Driver/aarch64-sve-vector-bits.c
@@ -22,21 +22,6 @@
 // CHECK-2048: "-msve-vector-bits=2048"
 // CHECK-SCALABLE-NOT: "-msve-vector-bits=
 
-// Bail out if -msve-vector-bits is specified without SVE enabled
-// -
-// RUN: %clang -c %s -### -target aarch64-none-linux-gnu -msve-vector-bits=128 \
-// RUN:  2>&1 | FileCheck --check-prefix=CHECK-NO-SVE-ERROR %s
-// RUN: %clang -c %s -### -target aarch64-none-linux-gnu -msve-vector-bits=256 \
-// RUN:  2>&1 | FileCheck --check-prefix=CHECK-NO-SVE-ERROR %s
-// RUN: %clang -c %s -### -target aarch64-none-linux-gnu -msve-vector-bits=512 \
-// RUN:  2>&1 | FileCheck --check-prefix=CHECK-NO-SVE-ERROR %s
-// RUN: %clang -c %s -### -target aarch64-none-linux-gnu -msve-vector-bits=1024 \
-// RUN:  2>&1 | FileCheck --check-prefix=CHECK-NO-SVE-ERROR %s
-// RUN: %clang -c %s -### -target aarch64-none-linux-gnu -msve-vector-bits=2048 \
-// RUN:  2>&1 | FileCheck --check-prefix=CHECK-NO-SVE-ERROR %s
-
-// CHECK-NO-SVE-ERROR: error: '-msve-vector-bits' is not supported without SVE enabled
-
 // Error out if an unsupported value is passed to -msve-vector-bits.
 // -
 // RUN: %clang -c %s -### -target aarch64-none-linux-gnu -march=armv8-a+sve \
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -7785,7 +7785,8 @@
   // not to need a separate attribute)
   if (!S.Context.getTargetInfo().hasFeature("neon") &&
   !S.Context.getTargetInfo().hasFeature("mve")) {
-S.Diag(Attr.getLoc(), diag::err_attribute_unsupported) << Attr;
+S.Diag(Attr.getLoc(), diag::err_attribute_unsupported)
+<< Attr << "'neon' or 'mve'";
 Attr.setInvalid();
 return;
   }
@@ -7828,7 +7829,8 @@
Sema ) {
   // Target must have SVE.
   if (!S.Context.getTargetInfo().hasFeature("sve")) {
-S.Diag(Attr.getLoc(), diag::err_attribute_unsupported) << Attr;
+S.Diag(Attr.getLoc(), diag::err_attribute_unsupported)
+<< Attr << "'sve'";
 Attr.setInvalid();
 return;
   }
Index: clang/lib/Driver/ToolChains/Arch/AArch64.cpp
===
--- clang/lib/Driver/ToolChains/Arch/AArch64.cpp
+++ clang/lib/Driver/ToolChains/Arch/AArch64.cpp
@@ -376,12 +376,6 @@
   if (V8_6Pos != std::end(Features))
 V8_6Pos = Features.insert(std::next(V8_6Pos), {"+i8mm", "+bf16"});
 
-  bool HasSve = llvm::is_contained(Features, "+sve");
-  // -msve-vector-bits= flag is valid only if SVE is enabled.
-  if (Args.hasArg(options::OPT_msve_vector_bits_EQ))
-if 

[PATCH] D92487: [AArch64][Driver][SVE] Push missing SVE feature error from driver to frontend

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm created this revision.
Herald added subscribers: cfe-commits, psnobl, kristof.beyls, tschuett.
Herald added a reviewer: efriedma.
Herald added a project: clang.
peterwaller-arm requested review of this revision.

... and give more guidance to users.

If specifying -msve-vector-bits on a non-SVE target, clang would say:

  error: '-msve-vector-bits' is not supported without SVE enabled

1. The driver lacks logic for "implied features". This would result in this 
error being raised for -march=...+sve2, even though +sve2 implies +sve.

2. Feature implication is well modelled in LLVM, so push the error down the 
stack.

3. Hint to the user what flag they need to consider setting.

Now clang fails later, when the feature is used, saying:

  aarch64-sve-vector-bits.c:42:41: error: 'arm_sve_vector_bits' attribute is 
not supported on targets missing 'sve' feature flag; specify an appropriate 
-march= or -mcpu=
  typedef svint32_t noflag __attribute__((arm_sve_vector_bits(256)));

Move clang/test/Sema/{neon => arm}-vector-types-support.c and put tests for
this warning together in one place.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92487

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Driver/aarch64-sve-vector-bits.c
  clang/test/Sema/arm-vector-types-support.c
  clang/test/Sema/neon-vector-types-support.c

Index: clang/test/Sema/neon-vector-types-support.c
===
--- clang/test/Sema/neon-vector-types-support.c
+++ /dev/null
@@ -1,4 +0,0 @@
-// RUN: %clang_cc1 %s -triple armv7 -fsyntax-only -verify
-
-typedef __attribute__((neon_vector_type(2))) int int32x2_t; // expected-error{{'neon_vector_type' attribute is not supported for this target}}
-typedef __attribute__((neon_polyvector_type(16))) short poly8x16_t; // expected-error{{'neon_polyvector_type' attribute is not supported for this target}}
Index: clang/test/Sema/arm-vector-types-support.c
===
--- /dev/null
+++ clang/test/Sema/arm-vector-types-support.c
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 %s -triple armv7 -fsyntax-only -verify
+
+typedef __attribute__((neon_vector_type(2))) int int32x2_t; // expected-error{{'neon_vector_type' attribute is not supported on targets missing 'neon' or 'mve' feature flag; specify an appropriate -march= or -mcpu=}}
+typedef __attribute__((neon_polyvector_type(16))) short poly8x16_t; // expected-error{{'neon_polyvector_type' attribute is not supported on targets missing 'neon' or 'mve' feature flag; specify an appropriate -march= or -mcpu=}}
+typedef __attribute__((arm_sve_vector_bits(256))) void nosveflag; // expected-error{{'arm_sve_vector_bits' attribute is not supported on targets missing 'sve' feature flag; specify an appropriate -march= or -mcpu=}}
Index: clang/test/Driver/aarch64-sve-vector-bits.c
===
--- clang/test/Driver/aarch64-sve-vector-bits.c
+++ clang/test/Driver/aarch64-sve-vector-bits.c
@@ -22,21 +22,6 @@
 // CHECK-2048: "-msve-vector-bits=2048"
 // CHECK-SCALABLE-NOT: "-msve-vector-bits=
 
-// Bail out if -msve-vector-bits is specified without SVE enabled
-// -
-// RUN: %clang -c %s -### -target aarch64-none-linux-gnu -msve-vector-bits=128 \
-// RUN:  2>&1 | FileCheck --check-prefix=CHECK-NO-SVE-ERROR %s
-// RUN: %clang -c %s -### -target aarch64-none-linux-gnu -msve-vector-bits=256 \
-// RUN:  2>&1 | FileCheck --check-prefix=CHECK-NO-SVE-ERROR %s
-// RUN: %clang -c %s -### -target aarch64-none-linux-gnu -msve-vector-bits=512 \
-// RUN:  2>&1 | FileCheck --check-prefix=CHECK-NO-SVE-ERROR %s
-// RUN: %clang -c %s -### -target aarch64-none-linux-gnu -msve-vector-bits=1024 \
-// RUN:  2>&1 | FileCheck --check-prefix=CHECK-NO-SVE-ERROR %s
-// RUN: %clang -c %s -### -target aarch64-none-linux-gnu -msve-vector-bits=2048 \
-// RUN:  2>&1 | FileCheck --check-prefix=CHECK-NO-SVE-ERROR %s
-
-// CHECK-NO-SVE-ERROR: error: '-msve-vector-bits' is not supported without SVE enabled
-
 // Error out if an unsupported value is passed to -msve-vector-bits.
 // -
 // RUN: %clang -c %s -### -target aarch64-none-linux-gnu -march=armv8-a+sve \
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -7785,7 +7785,8 @@
   // not to need a separate attribute)
   if (!S.Context.getTargetInfo().hasFeature("neon") &&
   !S.Context.getTargetInfo().hasFeature("mve")) {
-S.Diag(Attr.getLoc(), diag::err_attribute_unsupported) << Attr;
+S.Diag(Attr.getLoc(), diag::err_attribute_unsupported)
+

[PATCH] D91262: [AArch64][SVE] Allow C-style casts between fixed-size and scalable vectors

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added inline comments.



Comment at: clang/lib/Sema/SemaCast.cpp:
   if (srcIsVector || destIsVector) {
+// We can bitcast between SVE VLATs and VLSTs, and vice-versa.
+if (Self.isValidSveBitcast(SrcType, DestType)) {

It's good to avoid use of pronouns such as "We" in comments like this, since a 
different reader might take a different interpretation of the word "We". Is 
"We" the software itself? Is it the company who wrote the condition? Better to 
rewrite it in a more direct way; in my suggestion it is written so that it's 
clearer the following statements are what allow it. Even better if you can 
include a reference to a spec indicating the background on why it is allowed.



Comment at: clang/lib/Sema/SemaCast.cpp:2762
+  // If either the src or dest are a vector, it's possible that we want to do 
an
+  // SVE bitcast. We can bitcast between SVE VLATs and VLSTs, and vice-versa.
+  if (SrcType->isVectorType() || DestType->isVectorType())

Another couple of uses of the word "we". Suggest taking Cullen's suggestion to 
combine the conditions and say:

"Allow bitcasting between compatible SVE vector types".



Comment at: clang/lib/Sema/SemaCast.cpp:2763
+  // SVE bitcast. We can bitcast between SVE VLATs and VLSTs, and vice-versa.
+  if (SrcType->isVectorType() || DestType->isVectorType())
+if (Self.isValidSveBitcast(SrcType, DestType)) {

c-rhodes wrote:
> I think braces are recommended on the outer if as well, see: 
> https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
> 
> Although I suppose it could also be written as:
> 
> ```
> if ((SrcType->isVectorType() || DestType->isVectorType()) &&
> Self.isValidSveBitcast(SrcType, DestType)) {
>   Kind = CK_BitCast;
>   return;
> }```
I don't understand why this is an || rather than an &&, please can you clarify? 
I would have expected they must both be vectors.



Comment at: clang/lib/Sema/SemaExpr.cpp:7200
 
+/// Are the two types SVE-bitcast-compatible types? I.e. can we bitcast from 
the
+/// first SVE type (e.g. an SVE VLAT) to the second type (e.g. an SVE VLST)?

Use of "we".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91262

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

[PATCH] D90956: [clang][SVE] Additional macros implied by `-msve-vector-bits=`.

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

Suggestion: Where referencing sections you could include the title "3.7.3.3. 
Behavior specific to SVE vectors" instead of "3.7.3.3" for example. I think 
this makes it easier to spot misreferences and gives future souls a better 
chance to understand the context without indirection.




Comment at: 
clang/test/CodeGen/aarch64-sve-acle-__ARM_FEATURE_SVE_PREDICATE_OPERATORS.cpp:19
+auto f4(pred x, pred y) { return x & y; } // Returns a pred
+#endif

Do we need tests for unary operators, assignment and comparison operators?
(Page 28, item 3, subitems, 1, 3, 4)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90956

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

[PATCH] D90956: [clang][SVE] Additional macros implied by `-msve-vector-bits=`.

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added inline comments.



Comment at: 
clang/test/CodeGen/aarch64-sve-acle-__ARM_FEATURE_SVE_VECTOR_OPERATORS-expected-error-on-address.c:6
+
+// Examples taken from section 3.7.3.3 of the SVE ACLE (Version
+// 00bet6) that can be found at




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90956

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

[PATCH] D91067: [AArch64][SVE] Support implicit lax vector conversions for SVE types

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

fixed_float64_t appears in the commit message but also is unused.




Comment at: clang/test/Sema/aarch64-sve-lax-vector-conversions.cpp:15
+typedef svint32_t fixed_int32_t FIXED_ATTR;
+typedef svint64_t fixed_int64_t FIXED_ATTR;
+

I can't see any uses of fixed_float64_t and fixed_int64_t?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91067

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

[PATCH] D63607: [clang][driver] Add basic --driver-mode=flang support for fortran

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

@thakis: I've marked the test unsupported in c75cd3c7f0f 
. 
Hopefully that makes your builder happy! I'll figure out what is going on and 
fix it.


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

https://reviews.llvm.org/D63607



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

[PATCH] D63607: [clang][driver] Add basic --driver-mode=flang support for fortran

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

I've thought about it for a moment and I'm currently at a loss to quickly 
explain why this would only fail on darwin. In my patch, the change to 
LookupTypeForExtension should prevent clang from reaching this state where it 
complains about a preprocessed input.

I don't have access to a darwin machine right now to dive in, understand and 
fix it.

I propose to mark the test `UNSUPPORTED: darwin` until more is understood about 
the breakage. I assume there is no issue with that?


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

https://reviews.llvm.org/D63607



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

[PATCH] D63607: [clang][driver] Add basic --driver-mode=flang support for fortran

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

@thakis: I found the thread at 
https://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20120430/057199.html

@thakis: I can't make quick sense of the failure from those logs alone. I would 
appreciate it very much to see the output of the `clang -###` run lines to see 
what's missing. It should be showing that it would invoke flang.


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

https://reviews.llvm.org/D63607



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

[PATCH] D63607: [clang][driver] Add basic --driver-mode=flang support for fortran

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

Thanks for chiming in.

> Sorry, I had missed the RfC, but it looks like there wasn't a lot of 
> discussion on it anyways.

Apologies @thakis - did I jump the gun? And if so, what could I have done 
differently?

> Adding fortran support to clang's driver has been suggested and decided 
> against before, see "[cfe-commits] [RFC and PATCH] Fortran"

Interesting, I had missed that conversation! Google and DDG fail to find it for 
me even quoting sentences out of it. Found it by wgetting the gzip mailing list 
archives and grepping them - is there a better way? :)

That conversation was in 2012, and since then flang has been accepted as a 
subproject to LLVM in "[llvm-dev] f18 is accepted as part of LLVM project! 
". Since the 
acceptance, the project has renamed to flang, and is in the process of actively 
being integrated into the llvm project.

As discussed in the RFC, I understand that libclangDriver was always intended 
to be a flexible driver used by other frontends, so in light of that, does the 
change still seem unreasonable? My expectation is that the footprint of fortran 
on libclangDriver should only be quite modest.


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

https://reviews.llvm.org/D63607



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

[PATCH] D63607: [clang][driver] Add basic --driver-mode=flang support for fortran

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

In D63607#1726895 , @teemperor wrote:

> Could we have the `clang/test/Driver/flang/flang.F90` and 
> `clang/test/Driver/flang/flang.f90` files in different directories please? As 
> macOS's FS is case-insensitive, those two files have the same path from macOS 
> perspective which is causing a whole bunch of issues (including breaking git 
> even with 'core.ignorecase').


This was fixed by Jeremy Morse in 6c0a160c2d33e54aecf1538bf7c85d8da92051be 
 - thanks.

I'm investigating the non-deterministic failure but don't have access to a mac 
to test.

@thakis is the flaky test resolved by 6c0a160c 
 or is it 
still outstanding?


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

https://reviews.llvm.org/D63607



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

[PATCH] D63607: [clang][driver] Add basic --driver-mode=flang support for fortran

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm closed this revision.
peterwaller-arm added a comment.

Submitted in 6bf55804924d5a1d902925ad080b1a2b57c5c75c 
.


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

https://reviews.llvm.org/D63607



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

[PATCH] D63607: [clang][driver] Add basic --driver-mode=flang support for fortran

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

Revoking my previous request: I now have commit access and I intend to submit 
this shortly.


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

https://reviews.llvm.org/D63607



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

[PATCH] D63607: [clang][driver] Add basic --driver-mode=flang support for fortran

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm marked an inline comment as done.
peterwaller-arm added a comment.

I'm still awaiting commit access, please can someone submit this on my behalf?


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

https://reviews.llvm.org/D63607



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

[PATCH] D63607: [clang][driver] Add basic --driver-mode=flang support for fortran

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm marked 2 inline comments as done.
peterwaller-arm added inline comments.



Comment at: clang/test/Driver/flang/flang-not-installed.f90:11
+! shell syntax.
+! UNSUPPORTED: windows
+

hfinkel wrote:
> I believe that you can write:
> 
>   REQUIRES: shell
> 
> for this.
I've removed this test for now because I discovered that setting PATH="" is not 
enough to make clang lose the flang binary, if the flang binary is in the same 
directory as clang (which will be likely once flang is in-tree).


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

https://reviews.llvm.org/D63607



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

[PATCH] D63607: [clang][driver] Add basic --driver-mode=flang support for fortran

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm updated this revision to Diff 226270.
peterwaller-arm added a comment.

I have rebased the patch for conflicts to master and all the tests are passing.

While doing so, I discovered that the test for flang-not-installed was not fit 
for purpose, because clang actually doesn't first check the PATH, it can find a 
flang binary which lives in the same directory as itself. I conclude that 
having such a test is more trouble than it is worth. Adding such a test would 
involve adding some questionable test-specific machinery. Therefore I've 
removed that test for now.

I'll leave the patch over the weekend in case anyone objects, otherwise I 
intend to submit early next week.


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

https://reviews.llvm.org/D63607

Files:
  clang/include/clang/Driver/Driver.h
  clang/include/clang/Driver/ToolChain.h
  clang/include/clang/Driver/Types.h
  clang/lib/Driver/CMakeLists.txt
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/ToolChains/Flang.h
  clang/lib/Driver/Types.cpp
  clang/test/Driver/flang/Inputs/one.f90
  clang/test/Driver/flang/Inputs/other.c
  clang/test/Driver/flang/Inputs/two.f90
  clang/test/Driver/flang/flang.F90
  clang/test/Driver/flang/flang.f90
  clang/test/Driver/flang/multiple-inputs-mixed.f90
  clang/test/Driver/flang/multiple-inputs.f90
  clang/test/Driver/fortran.f95
  clang/test/Driver/lit.local.cfg

Index: clang/test/Driver/lit.local.cfg
===
--- clang/test/Driver/lit.local.cfg
+++ clang/test/Driver/lit.local.cfg
@@ -1,4 +1,4 @@
-config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.f95',
+config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
'.cu', '.rs', '.cl', '.hip']
 config.substitutions = list(config.substitutions)
 config.substitutions.insert(0,
Index: clang/test/Driver/fortran.f95
===
--- clang/test/Driver/fortran.f95
+++ clang/test/Driver/fortran.f95
@@ -1,21 +1,22 @@
-// Check that the clang driver can invoke gcc to compile Fortran.
+! Check that the clang driver can invoke gcc to compile Fortran when in
+! --driver-mode=clang. This is legacy behaviour - see also --driver-mode=flang.
 
-// RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -c %s -### 2>&1 \
-// RUN:   | FileCheck --check-prefix=CHECK-OBJECT %s
-// CHECK-OBJECT: gcc
-// CHECK-OBJECT: "-c"
-// CHECK-OBJECT: "-x" "f95"
-// CHECK-OBJECT-NOT: cc1as
+! RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -c %s -### 2>&1 \
+! RUN:   | FileCheck --check-prefix=CHECK-OBJECT %s
+! CHECK-OBJECT: gcc
+! CHECK-OBJECT: "-c"
+! CHECK-OBJECT: "-x" "f95"
+! CHECK-OBJECT-NOT: cc1as
 
-// RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -S %s -### 2>&1 \
-// RUN:   | FileCheck --check-prefix=CHECK-ASM %s
-// CHECK-ASM: gcc
-// CHECK-ASM: "-S"
-// CHECK-ASM: "-x" "f95"
-// CHECK-ASM-NOT: cc1
+! RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -S %s -### 2>&1 \
+! RUN:   | FileCheck --check-prefix=CHECK-ASM %s
+! CHECK-ASM: gcc
+! CHECK-ASM: "-S"
+! CHECK-ASM: "-x" "f95"
+! CHECK-ASM-NOT: cc1
 
-// RUN: %clang -Wall -target x86_64-unknown-linux-gnu -integrated-as %s -o %t -### 2>&1 | FileCheck --check-prefix=CHECK-WARN %s
-// CHECK-WARN: gcc
-// CHECK-WARN-NOT: "-Wall"
-// CHECK-WARN: ld
-// CHECK-WARN-NOT: "-Wall"
+! RUN: %clang -Wall -target x86_64-unknown-linux-gnu -integrated-as %s -o %t -### 2>&1 | FileCheck --check-prefix=CHECK-WARN %s
+! CHECK-WARN: gcc
+! CHECK-WARN-NOT: "-Wall"
+! CHECK-WARN: ld
+! CHECK-WARN-NOT: "-Wall"
Index: clang/test/Driver/flang/multiple-inputs.f90
===
--- /dev/null
+++ clang/test/Driver/flang/multiple-inputs.f90
@@ -0,0 +1,7 @@
+! Check that flang driver can handle multiple inputs at once.
+
+! RUN: %clang --driver-mode=flang -### -fsyntax-only %S/Inputs/one.f90 %S/Inputs/two.f90 2>&1 | FileCheck --check-prefixes=CHECK-SYNTAX-ONLY %s
+! CHECK-SYNTAX-ONLY-LABEL: "{{[^"]*}}flang" "-fc1"
+! CHECK-SYNTAX-ONLY: "{{[^"]*}}/Inputs/one.f90"
+! CHECK-SYNTAX-ONLY-LABEL: "{{[^"]*}}flang" "-fc1"
+! CHECK-SYNTAX-ONLY: "{{[^"]*}}/Inputs/two.f90"
Index: clang/test/Driver/flang/multiple-inputs-mixed.f90
===
--- /dev/null
+++ clang/test/Driver/flang/multiple-inputs-mixed.f90
@@ -0,0 +1,7 @@
+! Check that flang can handle mixed C and fortran inputs.
+
+! RUN: %clang --driver-mode=flang -### -fsyntax-only %S/Inputs/one.f90 %S/Inputs/other.c 2>&1 | FileCheck --check-prefixes=CHECK-SYNTAX-ONLY %s
+! CHECK-SYNTAX-ONLY-LABEL: "{{[^"]*}}flang{{[^"/]*}}" "-fc1"
+! CHECK-SYNTAX-ONLY: "{{[^"]*}}/Inputs/one.f90"
+! CHECK-SYNTAX-ONLY-LABEL: "{{[^"]*}}clang{{[^"/]*}}" "-cc1"
+! CHECK-SYNTAX-ONLY: "{{[^"]*}}/Inputs/other.c"

[PATCH] D63607: [clang][driver] Add basic --driver-mode=flang support for fortran

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

Friendly ping to everybody watching. I'd like to get this in soon if possible.

Hal - do you think this is close to being accepted? Note that I consider this 
"the beginning" rather than "the end", since there will be more functionality 
to add piecewise before this is fully functional. In the meantime, since it is 
new functionality, it should not break anything.


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

https://reviews.llvm.org/D63607



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

[PATCH] D63607: [clang][driver] Add basic --driver-mode=flang support for fortran

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

Hi All, I'm going on leave for two weeks, returning October 14th. I can 
plausibly respond to comments until 14:00 UTC tomorrow (Thu 25th Sept).

As a quick note, another piece of the puzzle has been submitted for review at 
https://github.com/flang-compiler/f18/pull/759, which is the flang-side binary 
which will link to this code and default to running the driver in flang mode.


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

https://reviews.llvm.org/D63607



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

[PATCH] D63607: [clang][driver] Add basic --driver-mode=flang support for fortran

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added inline comments.



Comment at: clang/include/clang/Driver/Driver.h:69
+CLMode,
+FlangMode,
   } Mode;

kiranchandramohan wrote:
> Is the comma by choice?
It was not. Fixed.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:37
+  if (isa(JA)) {
+CmdArgs.push_back("-emit-obj");
+  } else if (isa(JA)) {

kiranchandramohan wrote:
> peterwaller-arm wrote:
> > richard.barton.arm wrote:
> > > F18 does not currently support these options that control the output like 
> > > -emit-llvm and -emit-obj so this code doesn't do anything sensible at 
> > > present. Would it not make more sense to add this later on once F18 or 
> > > llvm/flang grows support for such options?
> > I've removed them.
> Can it be removed from the Summary of this PR?
They have now been reintroduced after we found that it had an unpleasant 
failure mode. Better to implement them now, after all, and fail gracefully in 
the frontend.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:44
+if (JA.getType() == types::TY_Nothing) {
+  CmdArgs.push_back("-fsyntax-only");
+} else if (JA.getType() == types::TY_LLVM_IR ||

richard.barton.arm wrote:
> peterwaller-arm wrote:
> > richard.barton.arm wrote:
> > > Looks like the F18 option spelling for this is -fparse-only? Or maybe 
> > > -fdebug-semantics? I know the intention is to throw away the 'throwaway 
> > > driver' in F18, so perhaps you are preparing to replace this option 
> > > spelling in the throwaway driver with -fsyntax-only so this would 
> > > highlight that perhaps adding the code here before the F18 driver is 
> > > ready to accept it is not the right approach?
> > In the RFC, the intent expressed was to mimick gfortran and clang options. 
> > I am making a spelling choice here that I think it should be called 
> > -fsyntax-only, which matches those. I intend to make the `flang -fc1` side 
> > match in the near future.
> > 
> > -fdebug-* could be supported through an `-Xflang ...` option to pass debug 
> > flags through.
> OK - happy with this approach. So -fsyntax-only and -emit-ast will be the new 
> names for f18's -fdebug-semantics and -fdebug-dump-parse-tree I guess.
Agreed. They can still be changed later if necessary.



Comment at: clang/lib/Driver/Types.cpp:220
+
+  case TY_Fortran: case TY_PP_Fortran:
+return true;

kiranchandramohan wrote:
> Now that there is a 2018 standard, I am assuming that f18 and F18 are also 
> valid fortran extensions. Can these be added to the File types?
> 
> Also, should the TypeInfo file be extended to handle all Fortran file types? 
> ./include/clang/Driver/Types.def
> 
> Also, should we capture some information about the standards from the 
> filename extension?
My reading of the code is that both [fF]90 and [fF]95 both get turned into 
TY_Fortran/TY_PP_Fortran.

Both of these in the Types.def end up coerced to "f95", currently. I don't 
think the driver wants an entry in the Types.def for each fortran standard - it 
doesn't have them for other languages, for example, and I'm unsure what the 
benefit of that would be. The main purpose of that table as far as I see is to 
determine what phases need to run.

My feeling is that the name in the types table should be "fortran". I don't 
want to make that change in the scope of this PR, since it might be a breaking 
change. It looks to me like the name works its way out towards human eyeballs 
but is otherwise unused.

I would like to implement -std inference from the file extension and additional 
fortran filename extensions (f18 etc) in another change request.



Comment at: clang/lib/Driver/Types.cpp:220
+
+  case TY_Fortran: case TY_PP_Fortran:
+return true;

richard.barton.arm wrote:
> peterwaller-arm wrote:
> > kiranchandramohan wrote:
> > > Now that there is a 2018 standard, I am assuming that f18 and F18 are 
> > > also valid fortran extensions. Can these be added to the File types?
> > > 
> > > Also, should the TypeInfo file be extended to handle all Fortran file 
> > > types? ./include/clang/Driver/Types.def
> > > 
> > > Also, should we capture some information about the standards from the 
> > > filename extension?
> > My reading of the code is that both [fF]90 and [fF]95 both get turned into 
> > TY_Fortran/TY_PP_Fortran.
> > 
> > Both of these in the Types.def end up coerced to "f95", currently. I don't 
> > think the driver wants an entry in the Types.def for each fortran standard 
> > - it doesn't have them for other languages, for example, and I'm unsure 
> > what the benefit of that would be. The main purpose of that table as far as 
> > I see is to determine what phases need to run.
> > 
> > My feeling is that the name in the types table should be "fortran". I don't 
> > want to make that change in the scope of this PR, since it might be a 
> > breaking change. It looks 

[PATCH] D63607: [clang][driver] Add basic --driver-mode=flang support for fortran

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm updated this revision to Diff 220856.
peterwaller-arm marked 14 inline comments as done.
peterwaller-arm edited the summary of this revision.
peterwaller-arm added a comment.

Thanks everyone for your comments.

Changes since last patch:

- Reintroduce handling of (no phase arg specified), -S, -emit-llvm, etc.

  The code implementing it was reordered for clarity.

  After internal discussion we concluded it was preferable to land these now 
even though they are unimplemented on the frontend side, in order to avoid 
giving assertion errors for unimplemented behaviour.

  It is planned that the frontend will emit sensible errors for unimplemented 
behaviour.

- Fixes to the cover letter for --driver-mode=fortran => --driver-mode=flang

- Add a test (flang-not-installed.f90) which demonstrates the error message if 
clang is invoked with no flang available, by unsetting PATH. This is a 
non-"-###" test which also does not depend on the presence of flang.
  - Since it uses shell syntax, it is marked unsupported on windows.

- In response to a private comment, Flang mode now coerces TY_PP_Fortran to 
TY_Fortran. This:
  - Leaves the legacy behaviour unchanged
  - Ensures that we do not emit a warning when passing TY_PP_Fortran inputs to 
-E
  - Should ensure that TY_PP_Fortran is treated exactly as a fortran file
  - Is consistent with f18's intent to not have any notion of preprocessed 
sources
  - Now the flang.f90 and flang.F90  tests are 
identical except for the filename

- Various minor whitespace changes and documentation fixes.


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

https://reviews.llvm.org/D63607

Files:
  clang/include/clang/Driver/Driver.h
  clang/include/clang/Driver/ToolChain.h
  clang/include/clang/Driver/Types.h
  clang/lib/Driver/CMakeLists.txt
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/ToolChains/Flang.h
  clang/lib/Driver/Types.cpp
  clang/test/Driver/flang/Inputs/one.f90
  clang/test/Driver/flang/Inputs/other.c
  clang/test/Driver/flang/Inputs/two.f90
  clang/test/Driver/flang/flang-not-installed.f90
  clang/test/Driver/flang/flang.F90
  clang/test/Driver/flang/flang.f90
  clang/test/Driver/flang/multiple-inputs-mixed.f90
  clang/test/Driver/flang/multiple-inputs.f90
  clang/test/Driver/fortran.f95
  clang/test/Driver/lit.local.cfg

Index: clang/test/Driver/lit.local.cfg
===
--- clang/test/Driver/lit.local.cfg
+++ clang/test/Driver/lit.local.cfg
@@ -1,4 +1,4 @@
-config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.f95',
+config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
'.cu', '.rs', '.cl', '.hip']
 config.substitutions = list(config.substitutions)
 config.substitutions.insert(0,
Index: clang/test/Driver/fortran.f95
===
--- clang/test/Driver/fortran.f95
+++ clang/test/Driver/fortran.f95
@@ -1,21 +1,22 @@
-// Check that the clang driver can invoke gcc to compile Fortran.
+! Check that the clang driver can invoke gcc to compile Fortran when in
+! --driver-mode=clang. This is legacy behaviour - see also --driver-mode=flang.
 
-// RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -c %s -### 2>&1 \
-// RUN:   | FileCheck --check-prefix=CHECK-OBJECT %s
-// CHECK-OBJECT: gcc
-// CHECK-OBJECT: "-c"
-// CHECK-OBJECT: "-x" "f95"
-// CHECK-OBJECT-NOT: cc1as
+! RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -c %s -### 2>&1 \
+! RUN:   | FileCheck --check-prefix=CHECK-OBJECT %s
+! CHECK-OBJECT: gcc
+! CHECK-OBJECT: "-c"
+! CHECK-OBJECT: "-x" "f95"
+! CHECK-OBJECT-NOT: cc1as
 
-// RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -S %s -### 2>&1 \
-// RUN:   | FileCheck --check-prefix=CHECK-ASM %s
-// CHECK-ASM: gcc
-// CHECK-ASM: "-S"
-// CHECK-ASM: "-x" "f95"
-// CHECK-ASM-NOT: cc1
+! RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -S %s -### 2>&1 \
+! RUN:   | FileCheck --check-prefix=CHECK-ASM %s
+! CHECK-ASM: gcc
+! CHECK-ASM: "-S"
+! CHECK-ASM: "-x" "f95"
+! CHECK-ASM-NOT: cc1
 
-// RUN: %clang -Wall -target x86_64-unknown-linux-gnu -integrated-as %s -o %t -### 2>&1 | FileCheck --check-prefix=CHECK-WARN %s
-// CHECK-WARN: gcc
-// CHECK-WARN-NOT: "-Wall"
-// CHECK-WARN: ld
-// CHECK-WARN-NOT: "-Wall"
+! RUN: %clang -Wall -target x86_64-unknown-linux-gnu -integrated-as %s -o %t -### 2>&1 | FileCheck --check-prefix=CHECK-WARN %s
+! CHECK-WARN: gcc
+! CHECK-WARN-NOT: "-Wall"
+! CHECK-WARN: ld
+! CHECK-WARN-NOT: "-Wall"
Index: clang/test/Driver/flang/multiple-inputs.f90
===
--- /dev/null
+++ clang/test/Driver/flang/multiple-inputs.f90
@@ -0,0 +1,7 @@
+! Check that flang driver can handle multiple inputs at once.
+
+! RUN: %clang 

[PATCH] D63607: [clang][driver] Add basic --driver-mode=flang support for fortran

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm updated this revision to Diff 220681.
peterwaller-arm marked 6 inline comments as done.
peterwaller-arm added a comment.

- Fixed spurious comma
- Fixed incorrect comment and changed comment wrapping.


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

https://reviews.llvm.org/D63607

Files:
  clang/include/clang/Driver/Driver.h
  clang/include/clang/Driver/ToolChain.h
  clang/include/clang/Driver/Types.h
  clang/lib/Driver/CMakeLists.txt
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/ToolChains/Flang.h
  clang/lib/Driver/Types.cpp
  clang/test/Driver/flang/Inputs/one.f90
  clang/test/Driver/flang/Inputs/other.c
  clang/test/Driver/flang/Inputs/two.f90
  clang/test/Driver/flang/flang.F90
  clang/test/Driver/flang/flang.f90
  clang/test/Driver/flang/multiple-inputs-mixed.f90
  clang/test/Driver/flang/multiple-inputs.f90
  clang/test/Driver/fortran.f95
  clang/test/Driver/lit.local.cfg

Index: clang/test/Driver/lit.local.cfg
===
--- clang/test/Driver/lit.local.cfg
+++ clang/test/Driver/lit.local.cfg
@@ -1,4 +1,4 @@
-config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.f95',
+config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
'.cu', '.rs', '.cl', '.hip']
 config.substitutions = list(config.substitutions)
 config.substitutions.insert(0,
Index: clang/test/Driver/fortran.f95
===
--- clang/test/Driver/fortran.f95
+++ clang/test/Driver/fortran.f95
@@ -1,21 +1,22 @@
-// Check that the clang driver can invoke gcc to compile Fortran.
+! Check that the clang driver can invoke gcc to compile Fortran when in
+! --driver-mode=clang. This is legacy behaviour - see also --driver-mode=fortran.
 
-// RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -c %s -### 2>&1 \
-// RUN:   | FileCheck --check-prefix=CHECK-OBJECT %s
-// CHECK-OBJECT: gcc
-// CHECK-OBJECT: "-c"
-// CHECK-OBJECT: "-x" "f95"
-// CHECK-OBJECT-NOT: cc1as
+! RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -c %s -### 2>&1 \
+! RUN:   | FileCheck --check-prefix=CHECK-OBJECT %s
+! CHECK-OBJECT: gcc
+! CHECK-OBJECT: "-c"
+! CHECK-OBJECT: "-x" "f95"
+! CHECK-OBJECT-NOT: cc1as
 
-// RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -S %s -### 2>&1 \
-// RUN:   | FileCheck --check-prefix=CHECK-ASM %s
-// CHECK-ASM: gcc
-// CHECK-ASM: "-S"
-// CHECK-ASM: "-x" "f95"
-// CHECK-ASM-NOT: cc1
+! RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -S %s -### 2>&1 \
+! RUN:   | FileCheck --check-prefix=CHECK-ASM %s
+! CHECK-ASM: gcc
+! CHECK-ASM: "-S"
+! CHECK-ASM: "-x" "f95"
+! CHECK-ASM-NOT: cc1
 
-// RUN: %clang -Wall -target x86_64-unknown-linux-gnu -integrated-as %s -o %t -### 2>&1 | FileCheck --check-prefix=CHECK-WARN %s
-// CHECK-WARN: gcc
-// CHECK-WARN-NOT: "-Wall"
-// CHECK-WARN: ld
-// CHECK-WARN-NOT: "-Wall"
+! RUN: %clang -Wall -target x86_64-unknown-linux-gnu -integrated-as %s -o %t -### 2>&1 | FileCheck --check-prefix=CHECK-WARN %s
+! CHECK-WARN: gcc
+! CHECK-WARN-NOT: "-Wall"
+! CHECK-WARN: ld
+! CHECK-WARN-NOT: "-Wall"
Index: clang/test/Driver/flang/multiple-inputs.f90
===
--- /dev/null
+++ clang/test/Driver/flang/multiple-inputs.f90
@@ -0,0 +1,7 @@
+! Check that flang driver can handle multiple inputs at once.
+
+! RUN: %clang --driver-mode=flang -### -fsyntax-only %S/Inputs/one.f90 %S/Inputs/two.f90 2>&1 | FileCheck --check-prefixes=CHECK-SYNTAX-ONLY %s
+! CHECK-SYNTAX-ONLY-LABEL: "{{[^"]*}}flang" "-fc1"
+! CHECK-SYNTAX-ONLY: "{{[^"]*}}/Inputs/one.f90"
+! CHECK-SYNTAX-ONLY-LABEL: "{{[^"]*}}flang" "-fc1"
+! CHECK-SYNTAX-ONLY: "{{[^"]*}}/Inputs/two.f90"
Index: clang/test/Driver/flang/multiple-inputs-mixed.f90
===
--- /dev/null
+++ clang/test/Driver/flang/multiple-inputs-mixed.f90
@@ -0,0 +1,7 @@
+! Check that flang can handle mixed C and fortran inputs.
+
+! RUN: %clang --driver-mode=flang -### -fsyntax-only %S/Inputs/one.f90 %S/Inputs/other.c 2>&1 | FileCheck --check-prefixes=CHECK-SYNTAX-ONLY %s
+! CHECK-SYNTAX-ONLY-LABEL: "{{[^"]*}}flang{{[^"/]*}}" "-fc1"
+! CHECK-SYNTAX-ONLY: "{{[^"]*}}/Inputs/one.f90"
+! CHECK-SYNTAX-ONLY-LABEL: "{{[^"]*}}clang{{[^"/]*}}" "-cc1"
+! CHECK-SYNTAX-ONLY: "{{[^"]*}}/Inputs/other.c"
Index: clang/test/Driver/flang/flang.f90
===
--- /dev/null
+++ clang/test/Driver/flang/flang.f90
@@ -0,0 +1,31 @@
+! Check that flang -fc1 is invoked when in --driver-mode=flang.
+
+! See also: flang.F90 - "an input which also requires preprocessing".
+
+! Test output types:
+! * -E
+! * -fsyntax-only
+
+! Most tests use ALL-LABEL to bracket the checks with the compiler invocation
+! and the source, which appear 

[PATCH] D63607: [clang][driver] Add basic --driver-mode=flang support for fortran

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm updated this revision to Diff 220646.
peterwaller-arm added a comment.

Updated comment for IsFlangMode.


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

https://reviews.llvm.org/D63607

Files:
  clang/include/clang/Driver/Driver.h
  clang/include/clang/Driver/ToolChain.h
  clang/include/clang/Driver/Types.h
  clang/lib/Driver/CMakeLists.txt
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/ToolChains/Flang.h
  clang/lib/Driver/Types.cpp
  clang/test/Driver/flang/Inputs/one.f90
  clang/test/Driver/flang/Inputs/other.c
  clang/test/Driver/flang/Inputs/two.f90
  clang/test/Driver/flang/flang.F90
  clang/test/Driver/flang/flang.f90
  clang/test/Driver/flang/multiple-inputs-mixed.f90
  clang/test/Driver/flang/multiple-inputs.f90
  clang/test/Driver/fortran.f95
  clang/test/Driver/lit.local.cfg

Index: clang/test/Driver/lit.local.cfg
===
--- clang/test/Driver/lit.local.cfg
+++ clang/test/Driver/lit.local.cfg
@@ -1,4 +1,4 @@
-config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.f95',
+config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
'.cu', '.rs', '.cl', '.hip']
 config.substitutions = list(config.substitutions)
 config.substitutions.insert(0,
Index: clang/test/Driver/fortran.f95
===
--- clang/test/Driver/fortran.f95
+++ clang/test/Driver/fortran.f95
@@ -1,21 +1,22 @@
-// Check that the clang driver can invoke gcc to compile Fortran.
+! Check that the clang driver can invoke gcc to compile Fortran when in
+! --driver-mode=clang. This is legacy behaviour - see also --driver-mode=fortran.
 
-// RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -c %s -### 2>&1 \
-// RUN:   | FileCheck --check-prefix=CHECK-OBJECT %s
-// CHECK-OBJECT: gcc
-// CHECK-OBJECT: "-c"
-// CHECK-OBJECT: "-x" "f95"
-// CHECK-OBJECT-NOT: cc1as
+! RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -c %s -### 2>&1 \
+! RUN:   | FileCheck --check-prefix=CHECK-OBJECT %s
+! CHECK-OBJECT: gcc
+! CHECK-OBJECT: "-c"
+! CHECK-OBJECT: "-x" "f95"
+! CHECK-OBJECT-NOT: cc1as
 
-// RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -S %s -### 2>&1 \
-// RUN:   | FileCheck --check-prefix=CHECK-ASM %s
-// CHECK-ASM: gcc
-// CHECK-ASM: "-S"
-// CHECK-ASM: "-x" "f95"
-// CHECK-ASM-NOT: cc1
+! RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -S %s -### 2>&1 \
+! RUN:   | FileCheck --check-prefix=CHECK-ASM %s
+! CHECK-ASM: gcc
+! CHECK-ASM: "-S"
+! CHECK-ASM: "-x" "f95"
+! CHECK-ASM-NOT: cc1
 
-// RUN: %clang -Wall -target x86_64-unknown-linux-gnu -integrated-as %s -o %t -### 2>&1 | FileCheck --check-prefix=CHECK-WARN %s
-// CHECK-WARN: gcc
-// CHECK-WARN-NOT: "-Wall"
-// CHECK-WARN: ld
-// CHECK-WARN-NOT: "-Wall"
+! RUN: %clang -Wall -target x86_64-unknown-linux-gnu -integrated-as %s -o %t -### 2>&1 | FileCheck --check-prefix=CHECK-WARN %s
+! CHECK-WARN: gcc
+! CHECK-WARN-NOT: "-Wall"
+! CHECK-WARN: ld
+! CHECK-WARN-NOT: "-Wall"
Index: clang/test/Driver/flang/multiple-inputs.f90
===
--- /dev/null
+++ clang/test/Driver/flang/multiple-inputs.f90
@@ -0,0 +1,7 @@
+! Check that flang driver can handle multiple inputs at once.
+
+! RUN: %clang --driver-mode=flang -### -fsyntax-only %S/Inputs/one.f90 %S/Inputs/two.f90 2>&1 | FileCheck --check-prefixes=CHECK-SYNTAX-ONLY %s
+! CHECK-SYNTAX-ONLY-LABEL: "{{[^"]*}}flang" "-fc1"
+! CHECK-SYNTAX-ONLY: "{{[^"]*}}/Inputs/one.f90"
+! CHECK-SYNTAX-ONLY-LABEL: "{{[^"]*}}flang" "-fc1"
+! CHECK-SYNTAX-ONLY: "{{[^"]*}}/Inputs/two.f90"
Index: clang/test/Driver/flang/multiple-inputs-mixed.f90
===
--- /dev/null
+++ clang/test/Driver/flang/multiple-inputs-mixed.f90
@@ -0,0 +1,7 @@
+! Check that flang can handle mixed C and fortran inputs.
+
+! RUN: %clang --driver-mode=flang -### -fsyntax-only %S/Inputs/one.f90 %S/Inputs/other.c 2>&1 | FileCheck --check-prefixes=CHECK-SYNTAX-ONLY %s
+! CHECK-SYNTAX-ONLY-LABEL: "{{[^"]*}}flang{{[^"/]*}}" "-fc1"
+! CHECK-SYNTAX-ONLY: "{{[^"]*}}/Inputs/one.f90"
+! CHECK-SYNTAX-ONLY-LABEL: "{{[^"]*}}clang{{[^"/]*}}" "-cc1"
+! CHECK-SYNTAX-ONLY: "{{[^"]*}}/Inputs/other.c"
Index: clang/test/Driver/flang/flang.f90
===
--- /dev/null
+++ clang/test/Driver/flang/flang.f90
@@ -0,0 +1,34 @@
+! Check that flang -fc1 is invoked when in --driver-mode=flang.
+
+! See also: flang.F90 - "an input which also requires preprocessing".
+
+! Test various output types:
+! * The default (-emit-obj)
+! * -fsyntax-only
+! * -emit-llvm
+! * -emit-llvm -S
+! * -S
+
+! Most tests use ALL-LABEL to bracket the checks with the compiler invocation and the source, which appear at the beginning and end.

[PATCH] D63607: [clang][driver] Add basic --driver-mode=flang support for fortran

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:4788
+bool Driver::ShouldUseFlangCompiler(const JobAction ) const {
+  // Say "no" if there is not exactly one input of a type flang understands.
+  if (JA.size() != 1 ||

richard.barton.arm wrote:
> This first clause surprised me. Is this a temporary measure or do we never 
> intend to support compiling more than one fortran file at once?
This function answers the question at the scope of a single JobAction. My 
understanding is that the compiler (as with clang -cc1) will be invoked once 
per input file.

This does not prevent multiple fortran files from being compiled with a single 
driver invocation.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:37
+  if (isa(JA)) {
+CmdArgs.push_back("-emit-obj");
+  } else if (isa(JA)) {

richard.barton.arm wrote:
> F18 does not currently support these options that control the output like 
> -emit-llvm and -emit-obj so this code doesn't do anything sensible at 
> present. Would it not make more sense to add this later on once F18 or 
> llvm/flang grows support for such options?
I've removed them.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:44
+if (JA.getType() == types::TY_Nothing) {
+  CmdArgs.push_back("-fsyntax-only");
+} else if (JA.getType() == types::TY_LLVM_IR ||

richard.barton.arm wrote:
> Looks like the F18 option spelling for this is -fparse-only? Or maybe 
> -fdebug-semantics? I know the intention is to throw away the 'throwaway 
> driver' in F18, so perhaps you are preparing to replace this option spelling 
> in the throwaway driver with -fsyntax-only so this would highlight that 
> perhaps adding the code here before the F18 driver is ready to accept it is 
> not the right approach?
In the RFC, the intent expressed was to mimick gfortran and clang options. I am 
making a spelling choice here that I think it should be called -fsyntax-only, 
which matches those. I intend to make the `flang -fc1` side match in the near 
future.

-fdebug-* could be supported through an `-Xflang ...` option to pass debug 
flags through.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:67
+  Args.AddAllArgs(CmdArgs, options::OPT_R_Group); // "Rpass-"" options 
passthrough.
+  Args.AddAllArgs(CmdArgs, options::OPT_gfortran_Group); // gfortran options 
passthrough.
+

richard.barton.arm wrote:
> Similarly to previous comment, do we want to be passing all gfortran options 
> through to F18 in the immediate term or even the long term? I don't think 
> there has been any agreement yet on what the options that F18 will support 
> are (although I agree that gfortran-like options would be most sensible and 
> in keeping with clang's philosophy)
I have deleted these options pass-throughs. I think you're right in general 
that we should only add options along with support for those things. The plan 
is now to add an OPT_flang_Group (or alike) later.



Comment at: clang/test/Driver/fortran.f95:1
-// Check that the clang driver can invoke gcc to compile Fortran.
+! Check that the clang driver can invoke gcc to compile Fortran.
 

richard.barton.arm wrote:
> ... when not in --driver-mode=fortran
Fixed.


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

https://reviews.llvm.org/D63607



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

[PATCH] D63607: [clang][driver] Add basic --driver-mode=flang support for fortran

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm updated this revision to Diff 220645.
peterwaller-arm marked 6 inline comments as done.
peterwaller-arm retitled this revision from "[clang][driver] Add basic 
--driver-mode=fortran support for flang" to "[clang][driver] Add basic 
--driver-mode=flang support for fortran".
peterwaller-arm added a comment.

Updated for review comments and spotted a couple of things myself.

Changes since last time:

- Removed various options which aren't yet implemented on the f18 side.
- --driver-mode=fortran is now --driver-mode=flang, which is more consistent 
with other modes.
- Added tests which show that multiple inputs (and mixed C+Fortran inputs) are 
supported by the driver.
- Updated comments.
- Removed a branch in getTool which didn't make sense.
- Updated commit message.


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

https://reviews.llvm.org/D63607

Files:
  clang/include/clang/Driver/Driver.h
  clang/include/clang/Driver/ToolChain.h
  clang/include/clang/Driver/Types.h
  clang/lib/Driver/CMakeLists.txt
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/ToolChains/Flang.h
  clang/lib/Driver/Types.cpp
  clang/test/Driver/flang/Inputs/one.f90
  clang/test/Driver/flang/Inputs/other.c
  clang/test/Driver/flang/Inputs/two.f90
  clang/test/Driver/flang/flang.F90
  clang/test/Driver/flang/flang.f90
  clang/test/Driver/flang/multiple-inputs-mixed.f90
  clang/test/Driver/flang/multiple-inputs.f90
  clang/test/Driver/fortran.f95
  clang/test/Driver/lit.local.cfg

Index: clang/test/Driver/lit.local.cfg
===
--- clang/test/Driver/lit.local.cfg
+++ clang/test/Driver/lit.local.cfg
@@ -1,4 +1,4 @@
-config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.f95',
+config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
'.cu', '.rs', '.cl', '.hip']
 config.substitutions = list(config.substitutions)
 config.substitutions.insert(0,
Index: clang/test/Driver/fortran.f95
===
--- clang/test/Driver/fortran.f95
+++ clang/test/Driver/fortran.f95
@@ -1,21 +1,22 @@
-// Check that the clang driver can invoke gcc to compile Fortran.
+! Check that the clang driver can invoke gcc to compile Fortran when in
+! --driver-mode=clang. This is legacy behaviour - see also --driver-mode=fortran.
 
-// RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -c %s -### 2>&1 \
-// RUN:   | FileCheck --check-prefix=CHECK-OBJECT %s
-// CHECK-OBJECT: gcc
-// CHECK-OBJECT: "-c"
-// CHECK-OBJECT: "-x" "f95"
-// CHECK-OBJECT-NOT: cc1as
+! RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -c %s -### 2>&1 \
+! RUN:   | FileCheck --check-prefix=CHECK-OBJECT %s
+! CHECK-OBJECT: gcc
+! CHECK-OBJECT: "-c"
+! CHECK-OBJECT: "-x" "f95"
+! CHECK-OBJECT-NOT: cc1as
 
-// RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -S %s -### 2>&1 \
-// RUN:   | FileCheck --check-prefix=CHECK-ASM %s
-// CHECK-ASM: gcc
-// CHECK-ASM: "-S"
-// CHECK-ASM: "-x" "f95"
-// CHECK-ASM-NOT: cc1
+! RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -S %s -### 2>&1 \
+! RUN:   | FileCheck --check-prefix=CHECK-ASM %s
+! CHECK-ASM: gcc
+! CHECK-ASM: "-S"
+! CHECK-ASM: "-x" "f95"
+! CHECK-ASM-NOT: cc1
 
-// RUN: %clang -Wall -target x86_64-unknown-linux-gnu -integrated-as %s -o %t -### 2>&1 | FileCheck --check-prefix=CHECK-WARN %s
-// CHECK-WARN: gcc
-// CHECK-WARN-NOT: "-Wall"
-// CHECK-WARN: ld
-// CHECK-WARN-NOT: "-Wall"
+! RUN: %clang -Wall -target x86_64-unknown-linux-gnu -integrated-as %s -o %t -### 2>&1 | FileCheck --check-prefix=CHECK-WARN %s
+! CHECK-WARN: gcc
+! CHECK-WARN-NOT: "-Wall"
+! CHECK-WARN: ld
+! CHECK-WARN-NOT: "-Wall"
Index: clang/test/Driver/flang/multiple-inputs.f90
===
--- /dev/null
+++ clang/test/Driver/flang/multiple-inputs.f90
@@ -0,0 +1,7 @@
+! Check that flang driver can handle multiple inputs at once.
+
+! RUN: %clang --driver-mode=flang -### -fsyntax-only %S/Inputs/one.f90 %S/Inputs/two.f90 2>&1 | FileCheck --check-prefixes=CHECK-SYNTAX-ONLY %s
+! CHECK-SYNTAX-ONLY-LABEL: "{{[^"]*}}flang" "-fc1"
+! CHECK-SYNTAX-ONLY: "{{[^"]*}}/Inputs/one.f90"
+! CHECK-SYNTAX-ONLY-LABEL: "{{[^"]*}}flang" "-fc1"
+! CHECK-SYNTAX-ONLY: "{{[^"]*}}/Inputs/two.f90"
Index: clang/test/Driver/flang/multiple-inputs-mixed.f90
===
--- /dev/null
+++ clang/test/Driver/flang/multiple-inputs-mixed.f90
@@ -0,0 +1,7 @@
+! Check that flang can handle mixed C and fortran inputs.
+
+! RUN: %clang --driver-mode=flang -### -fsyntax-only %S/Inputs/one.f90 %S/Inputs/other.c 2>&1 | FileCheck --check-prefixes=CHECK-SYNTAX-ONLY %s
+! CHECK-SYNTAX-ONLY-LABEL: "{{[^"]*}}flang{{[^"/]*}}" "-fc1"
+! CHECK-SYNTAX-ONLY: "{{[^"]*}}/Inputs/one.f90"
+! 

[PATCH] D63607: [clang][driver] Add basic --driver-mode=fortran support for flang

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm updated this revision to Diff 219666.
peterwaller-arm added a comment.

Fixed assertion message "Input output." => "Invalid output". The erroneous text 
came was copied from: 
https://github.com/llvm/llvm-project/blob/6b9df910d04fae62dacc22c1c84f66c0f126cde0/clang/lib/Driver/ToolChains/Clang.cpp#L3849


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

https://reviews.llvm.org/D63607

Files:
  clang/include/clang/Driver/Driver.h
  clang/include/clang/Driver/ToolChain.h
  clang/include/clang/Driver/Types.h
  clang/lib/Driver/CMakeLists.txt
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/ToolChains/Flang.h
  clang/lib/Driver/Types.cpp
  clang/test/Driver/flang/flang.F90
  clang/test/Driver/flang/flang.f90
  clang/test/Driver/fortran.f95
  clang/test/Driver/lit.local.cfg

Index: clang/test/Driver/lit.local.cfg
===
--- clang/test/Driver/lit.local.cfg
+++ clang/test/Driver/lit.local.cfg
@@ -1,4 +1,4 @@
-config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.f95',
+config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
'.cu', '.rs', '.cl', '.hip']
 config.substitutions = list(config.substitutions)
 config.substitutions.insert(0,
Index: clang/test/Driver/fortran.f95
===
--- clang/test/Driver/fortran.f95
+++ clang/test/Driver/fortran.f95
@@ -1,21 +1,21 @@
-// Check that the clang driver can invoke gcc to compile Fortran.
+! Check that the clang driver can invoke gcc to compile Fortran.
 
-// RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -c %s -### 2>&1 \
-// RUN:   | FileCheck --check-prefix=CHECK-OBJECT %s
-// CHECK-OBJECT: gcc
-// CHECK-OBJECT: "-c"
-// CHECK-OBJECT: "-x" "f95"
-// CHECK-OBJECT-NOT: cc1as
+! RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -c %s -### 2>&1 \
+! RUN:   | FileCheck --check-prefix=CHECK-OBJECT %s
+! CHECK-OBJECT: gcc
+! CHECK-OBJECT: "-c"
+! CHECK-OBJECT: "-x" "f95"
+! CHECK-OBJECT-NOT: cc1as
 
-// RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -S %s -### 2>&1 \
-// RUN:   | FileCheck --check-prefix=CHECK-ASM %s
-// CHECK-ASM: gcc
-// CHECK-ASM: "-S"
-// CHECK-ASM: "-x" "f95"
-// CHECK-ASM-NOT: cc1
+! RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -S %s -### 2>&1 \
+! RUN:   | FileCheck --check-prefix=CHECK-ASM %s
+! CHECK-ASM: gcc
+! CHECK-ASM: "-S"
+! CHECK-ASM: "-x" "f95"
+! CHECK-ASM-NOT: cc1
 
-// RUN: %clang -Wall -target x86_64-unknown-linux-gnu -integrated-as %s -o %t -### 2>&1 | FileCheck --check-prefix=CHECK-WARN %s
-// CHECK-WARN: gcc
-// CHECK-WARN-NOT: "-Wall"
-// CHECK-WARN: ld
-// CHECK-WARN-NOT: "-Wall"
+! RUN: %clang -Wall -target x86_64-unknown-linux-gnu -integrated-as %s -o %t -### 2>&1 | FileCheck --check-prefix=CHECK-WARN %s
+! CHECK-WARN: gcc
+! CHECK-WARN-NOT: "-Wall"
+! CHECK-WARN: ld
+! CHECK-WARN-NOT: "-Wall"
Index: clang/test/Driver/flang/flang.f90
===
--- /dev/null
+++ clang/test/Driver/flang/flang.f90
@@ -0,0 +1,56 @@
+! Check that flang -fc1 is invoked when in --driver-mode=fortran.
+
+! See also: flang.F90 - "an input which also requires preprocessing".
+
+! Test various output types:
+! * The default (-emit-obj)
+! * -fsyntax-only
+! * -emit-llvm
+! * -emit-llvm -S
+! * -S
+
+! Most tests use ALL-LABEL to bracket the checks with the compiler invocation and the source, which appear at the beginning and end.
+! Use of "...-DAG" allows the order of options to change within that bracket.
+
+
+! All invocations should begin with flang -fc1, consume up to here.
+! ALL-LABEL: "{{[^"]*}}flang" "-fc1"
+
+! RUN: %clang --driver-mode=fortran -### %s 2>&1 | FileCheck --check-prefixes=ALL,CHECK-PLAIN %s
+! CHECK-PLAIN-DAG: "-triple"
+! CHECK-PLAIN-DAG: "-emit-obj"
+! CHECK-PLAIN-DAG: "-o" "{{[^"]*}}.o"
+
+! RUN: %clang --driver-mode=fortran -emit-ast-### %s 2>&1 | FileCheck --check-prefixes=ALL,CHECK-EMIT-AST %s
+! CHECK-EMIT-AST-DAG: "-triple"
+! CHECK-EMIT-AST-DAG: "-emit-ast"
+! CHECK-EMIT-AST-DAG: "-o" "{{[^"]*}}.ast"
+
+! RUN: %clang --driver-mode=fortran -fsyntax-only   -### %s 2>&1 | FileCheck --check-prefixes=ALL,CHECK-SYNTAX-ONLY %s
+! CHECK-SYNTAX-ONLY-NOT: "-o"
+! CHECK-SYNTAX-ONLY-DAG: "-fsyntax-only"
+
+! RUN: %clang --driver-mode=fortran -emit-llvm  -### %s 2>&1 | FileCheck --check-prefixes=ALL,CHECK-EMIT-LLVM %s
+! CHECK-EMIT-LLVM-NOT: "-emit-llvm"
+! CHECK-EMIT-LLVM-DAG: "-emit-llvm-uselists"
+! CHECK-EMIT-LLVM-DAG: "-emit-llvm-bc"
+! CHECK-EMIT-LLVM-DAG: "-o" "{{[^"]*}}.bc"
+
+! RUN: %clang --driver-mode=fortran -emit-llvm -S   -### %s 2>&1 | FileCheck --check-prefixes=ALL,CHECK-EMIT-LLVM-S %s
+! CHECK-EMIT-LLVM-S-DAG: "-emit-llvm"
+! CHECK-EMIT-LLVM-S-DAG: "-o" "{{[^"]*}}.ll"
+
+! RUN: 

[PATCH] D63607: [clang][driver] Add basic --driver-mode=fortran support for flang

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm updated this revision to Diff 219538.
peterwaller-arm retitled this revision from "[clang][driver] Prototype  
--driver-mode=fortran support for new flang" to "[clang][driver] Add basic 
--driver-mode=fortran support for flang".
peterwaller-arm added a comment.

I updated this "prototype" revision into the real thing I would like to submit. 
Please let me know if that was the wrong thing to do and I will resubmit it.

This is an initial implementation which naturally doesn't implement very much. 
More will come in future patches.


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

https://reviews.llvm.org/D63607

Files:
  clang/include/clang/Driver/Driver.h
  clang/include/clang/Driver/ToolChain.h
  clang/include/clang/Driver/Types.h
  clang/lib/Driver/CMakeLists.txt
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/ToolChains/Flang.h
  clang/lib/Driver/Types.cpp
  clang/test/Driver/flang/flang.F90
  clang/test/Driver/flang/flang.f90
  clang/test/Driver/fortran.f95
  clang/test/Driver/lit.local.cfg

Index: clang/test/Driver/lit.local.cfg
===
--- clang/test/Driver/lit.local.cfg
+++ clang/test/Driver/lit.local.cfg
@@ -1,4 +1,4 @@
-config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.f95',
+config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
'.cu', '.rs', '.cl', '.hip']
 config.substitutions = list(config.substitutions)
 config.substitutions.insert(0,
Index: clang/test/Driver/fortran.f95
===
--- clang/test/Driver/fortran.f95
+++ clang/test/Driver/fortran.f95
@@ -1,21 +1,21 @@
-// Check that the clang driver can invoke gcc to compile Fortran.
+! Check that the clang driver can invoke gcc to compile Fortran.
 
-// RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -c %s -### 2>&1 \
-// RUN:   | FileCheck --check-prefix=CHECK-OBJECT %s
-// CHECK-OBJECT: gcc
-// CHECK-OBJECT: "-c"
-// CHECK-OBJECT: "-x" "f95"
-// CHECK-OBJECT-NOT: cc1as
+! RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -c %s -### 2>&1 \
+! RUN:   | FileCheck --check-prefix=CHECK-OBJECT %s
+! CHECK-OBJECT: gcc
+! CHECK-OBJECT: "-c"
+! CHECK-OBJECT: "-x" "f95"
+! CHECK-OBJECT-NOT: cc1as
 
-// RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -S %s -### 2>&1 \
-// RUN:   | FileCheck --check-prefix=CHECK-ASM %s
-// CHECK-ASM: gcc
-// CHECK-ASM: "-S"
-// CHECK-ASM: "-x" "f95"
-// CHECK-ASM-NOT: cc1
+! RUN: %clang -target x86_64-unknown-linux-gnu -integrated-as -S %s -### 2>&1 \
+! RUN:   | FileCheck --check-prefix=CHECK-ASM %s
+! CHECK-ASM: gcc
+! CHECK-ASM: "-S"
+! CHECK-ASM: "-x" "f95"
+! CHECK-ASM-NOT: cc1
 
-// RUN: %clang -Wall -target x86_64-unknown-linux-gnu -integrated-as %s -o %t -### 2>&1 | FileCheck --check-prefix=CHECK-WARN %s
-// CHECK-WARN: gcc
-// CHECK-WARN-NOT: "-Wall"
-// CHECK-WARN: ld
-// CHECK-WARN-NOT: "-Wall"
+! RUN: %clang -Wall -target x86_64-unknown-linux-gnu -integrated-as %s -o %t -### 2>&1 | FileCheck --check-prefix=CHECK-WARN %s
+! CHECK-WARN: gcc
+! CHECK-WARN-NOT: "-Wall"
+! CHECK-WARN: ld
+! CHECK-WARN-NOT: "-Wall"
Index: clang/test/Driver/flang/flang.f90
===
--- /dev/null
+++ clang/test/Driver/flang/flang.f90
@@ -0,0 +1,56 @@
+! Check that flang -fc1 is invoked when in --driver-mode=fortran.
+
+! See also: flang.F90 - "an input which also requires preprocessing".
+
+! Test various output types:
+! * The default (-emit-obj)
+! * -fsyntax-only
+! * -emit-llvm
+! * -emit-llvm -S
+! * -S
+
+! Most tests use ALL-LABEL to bracket the checks with the compiler invocation and the source, which appear at the beginning and end.
+! Use of "...-DAG" allows the order of options to change within that bracket.
+
+
+! All invocations should begin with flang -fc1, consume up to here.
+! ALL-LABEL: "{{[^"]*}}flang" "-fc1"
+
+! RUN: %clang --driver-mode=fortran -### %s 2>&1 | FileCheck --check-prefixes=ALL,CHECK-PLAIN %s
+! CHECK-PLAIN-DAG: "-triple"
+! CHECK-PLAIN-DAG: "-emit-obj"
+! CHECK-PLAIN-DAG: "-o" "{{[^"]*}}.o"
+
+! RUN: %clang --driver-mode=fortran -emit-ast-### %s 2>&1 | FileCheck --check-prefixes=ALL,CHECK-EMIT-AST %s
+! CHECK-EMIT-AST-DAG: "-triple"
+! CHECK-EMIT-AST-DAG: "-emit-ast"
+! CHECK-EMIT-AST-DAG: "-o" "{{[^"]*}}.ast"
+
+! RUN: %clang --driver-mode=fortran -fsyntax-only   -### %s 2>&1 | FileCheck --check-prefixes=ALL,CHECK-SYNTAX-ONLY %s
+! CHECK-SYNTAX-ONLY-NOT: "-o"
+! CHECK-SYNTAX-ONLY-DAG: "-fsyntax-only"
+
+! RUN: %clang --driver-mode=fortran -emit-llvm  -### %s 2>&1 | FileCheck --check-prefixes=ALL,CHECK-EMIT-LLVM %s
+! CHECK-EMIT-LLVM-NOT: "-emit-llvm"
+! CHECK-EMIT-LLVM-DAG: "-emit-llvm-uselists"
+! CHECK-EMIT-LLVM-DAG: "-emit-llvm-bc"
+! CHECK-EMIT-LLVM-DAG: "-o" 

[PATCH] D63607: [DO NOT SUBMIT] [clang][driver] Prototype --driver-mode=fortran support for new flang

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm updated this revision to Diff 206172.
peterwaller-arm added a comment.

Include full context.


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

https://reviews.llvm.org/D63607

Files:
  clang/include/clang/Driver/Driver.h
  clang/include/clang/Driver/ToolChain.h
  clang/include/clang/Driver/Types.h
  clang/lib/Driver/CMakeLists.txt
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/ToolChains/Flang.h
  clang/lib/Driver/Types.cpp
  clang/tools/driver/CMakeLists.txt
  clang/tools/driver/driver.cpp

Index: clang/tools/driver/driver.cpp
===
--- clang/tools/driver/driver.cpp
+++ clang/tools/driver/driver.cpp
@@ -305,12 +305,24 @@
 
 static int ExecuteCC1Tool(ArrayRef argv, StringRef Tool) {
   void *GetExecutablePathVP = (void *)(intptr_t) GetExecutablePath;
-  if (Tool == "")
+  // DONOTSUBMIT(peterwaller-arm): Please note that changes to this logic are
+  // only to facilitate demonstrating that -fc1 is being invoked, and will not
+  // be submitted in a future implementation.
+  if (Tool == "-cc1")
 return cc1_main(argv.slice(2), argv[0], GetExecutablePathVP);
-  if (Tool == "as")
+  if (Tool == "-cc1as")
 return cc1as_main(argv.slice(2), argv[0], GetExecutablePathVP);
-  if (Tool == "gen-reproducer")
+  if (Tool == "-cc1gen-reproducer")
 return cc1gen_reproducer_main(argv.slice(2), argv[0], GetExecutablePathVP);
+  if (Tool == "-fc1") {
+llvm::errs() << "invoked flang frontend: ";
+for (auto arg : argv) {
+  llvm::errs() << arg << " ";
+}
+llvm::errs() << "\n";
+return 1;
+  }
+
 
   // Reject unknown tools.
   llvm::errs() << "error: unknown integrated tool '" << Tool << "'. "
@@ -372,13 +384,18 @@
   // file.
   auto FirstArg = std::find_if(argv.begin() + 1, argv.end(),
[](const char *A) { return A != nullptr; });
-  if (FirstArg != argv.end() && StringRef(*FirstArg).startswith("-cc1")) {
+  // DONOTSUBMIT(peterwaller-arm): Please note that changes to this logic are
+  // only to facilitate demonstrating that -fc1 is being invoked, and will not
+  // be submitted in a future implementation.
+  if (FirstArg != argv.end() && (
+StringRef(*FirstArg).startswith("-cc1") || StringRef(*FirstArg).startswith("-fc1")
+)) {
 // If -cc1 came from a response file, remove the EOL sentinels.
 if (MarkEOLs) {
   auto newEnd = std::remove(argv.begin(), argv.end(), nullptr);
   argv.resize(newEnd - argv.begin());
 }
-return ExecuteCC1Tool(argv, argv[1] + 4);
+return ExecuteCC1Tool(argv, argv[1]);
   }
 
   bool CanonicalPrefixes = true;
Index: clang/tools/driver/CMakeLists.txt
===
--- clang/tools/driver/CMakeLists.txt
+++ clang/tools/driver/CMakeLists.txt
@@ -63,7 +63,7 @@
 add_dependencies(clang clang-resource-headers)
 
 if(NOT CLANG_LINKS_TO_CREATE)
-  set(CLANG_LINKS_TO_CREATE clang++ clang-cl clang-cpp)
+  set(CLANG_LINKS_TO_CREATE clang++ clang-cl clang-cpp flang)
 endif()
 
 foreach(link ${CLANG_LINKS_TO_CREATE})
Index: clang/lib/Driver/Types.cpp
===
--- clang/lib/Driver/Types.cpp
+++ clang/lib/Driver/Types.cpp
@@ -187,6 +187,16 @@
   }
 }
 
+bool types::isFortran(ID Id) {
+  switch (Id) {
+  default:
+return false;
+
+  case TY_Fortran: case TY_PP_Fortran:
+return true;
+  }
+}
+
 bool types::isSrcFile(ID Id) {
   return Id != TY_Object && getPreprocessedType(Id) != TY_INVALID;
 }
Index: clang/lib/Driver/ToolChains/Flang.h
===
--- /dev/null
+++ clang/lib/Driver/ToolChains/Flang.h
@@ -0,0 +1,48 @@
+//===--- Flang.h - Flang Tool and ToolChain Implementations -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_FLANG_H
+#define LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_FLANG_H
+
+#include "MSVC.h"
+#include "clang/Basic/DebugInfoOptions.h"
+#include "clang/Driver/Driver.h"
+#include "clang/Driver/Tool.h"
+#include "clang/Driver/Types.h"
+#include "llvm/ADT/Triple.h"
+#include "llvm/Option/Option.h"
+#include "llvm/Support/raw_ostream.h"
+
+namespace clang {
+namespace driver {
+
+namespace tools {
+
+/// Flang compiler tool.
+class LLVM_LIBRARY_VISIBILITY Flang : public Tool {
+public:
+  Flang(const ToolChain );
+  ~Flang() override;
+
+  bool hasGoodDiagnostics() const override { return true; }
+  bool hasIntegratedAssembler() const override { return true; }
+  bool hasIntegratedCPP() const override { return true; }
+  bool canEmitIR() const override { return 

[PATCH] D63607: [DO NOT SUBMIT] [clang][driver] Prototype --driver-mode=fortran support for new flang

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added a comment.

Note: In case you see this early, the email isn't yet sent to the list. I'll 
link it here when it is, likely tomorrow.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63607



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

[PATCH] D63607: [DO NOT SUBMIT] [clang][driver] Prototype --driver-mode=fortran support for new flang

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm created this revision.
Herald added subscribers: cfe-commits, kadircet, ilya-biryukov, mgorny.
Herald added a project: clang.

This is an early prototype, not intended for full review until after the
general approach is agreed. The purpose is to stimulate discussion on
the overall approach. Please see discussion in RFC posted to cfe-dev on
2019-06-20 titled "Adding a fortran mode to the clang driver for flang".

This patch adds a new Fortran mode. When in fortran mode, the driver
will invoke flang instead of falling back to the GCC toolchain as it
would otherwise do.

It is intended that a soon to be implemented binary in the flang project
will import libclangDriver and run the clang driver in the new fortran
mode.

Along with the new mode comes a new ToolChains/Flang.cpp. As the flang
frontend option parser is still prototypical, the implementation inside
this file is not yet fleshed out. It's a straw man.

Additionally, this patch makes "bin/flang" in the clang project, a
symlink to clang. This is only there to support trying the patch out
now, and will be removed when the real thing lands.

Likewise, the patch adds a new "-fc1" by analogy with "-cc1" and tweaks
the logic to facilitate that. This change is only to demonstrate the
overall effect, and will not be present in the real patch. The action of
-fc1 is just to print its command line arguments at the moment, which is
redundant with `-###` anyway. I just wanted to see it invoke a stub
flang with my own eyes.


Repository:
  rC Clang

https://reviews.llvm.org/D63607

Files:
  clang/include/clang/Driver/Driver.h
  clang/include/clang/Driver/ToolChain.h
  clang/include/clang/Driver/Types.h
  clang/lib/Driver/CMakeLists.txt
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/ToolChains/Flang.h
  clang/lib/Driver/Types.cpp
  clang/tools/driver/CMakeLists.txt
  clang/tools/driver/driver.cpp

Index: clang/tools/driver/driver.cpp
===
--- clang/tools/driver/driver.cpp
+++ clang/tools/driver/driver.cpp
@@ -305,12 +305,24 @@
 
 static int ExecuteCC1Tool(ArrayRef argv, StringRef Tool) {
   void *GetExecutablePathVP = (void *)(intptr_t) GetExecutablePath;
-  if (Tool == "")
+  // DONOTSUBMIT(peterwaller-arm): Please note that changes to this logic are
+  // only to facilitate demonstrating that -fc1 is being invoked, and will not
+  // be submitted in a future implementation.
+  if (Tool == "-cc1")
 return cc1_main(argv.slice(2), argv[0], GetExecutablePathVP);
-  if (Tool == "as")
+  if (Tool == "-cc1as")
 return cc1as_main(argv.slice(2), argv[0], GetExecutablePathVP);
-  if (Tool == "gen-reproducer")
+  if (Tool == "-cc1gen-reproducer")
 return cc1gen_reproducer_main(argv.slice(2), argv[0], GetExecutablePathVP);
+  if (Tool == "-fc1") {
+llvm::errs() << "invoked flang frontend: ";
+for (auto arg : argv) {
+  llvm::errs() << arg << " ";
+}
+llvm::errs() << "\n";
+return 1;
+  }
+
 
   // Reject unknown tools.
   llvm::errs() << "error: unknown integrated tool '" << Tool << "'. "
@@ -372,13 +384,18 @@
   // file.
   auto FirstArg = std::find_if(argv.begin() + 1, argv.end(),
[](const char *A) { return A != nullptr; });
-  if (FirstArg != argv.end() && StringRef(*FirstArg).startswith("-cc1")) {
+  // DONOTSUBMIT(peterwaller-arm): Please note that changes to this logic are
+  // only to facilitate demonstrating that -fc1 is being invoked, and will not
+  // be submitted in a future implementation.
+  if (FirstArg != argv.end() && (
+StringRef(*FirstArg).startswith("-cc1") || StringRef(*FirstArg).startswith("-fc1")
+)) {
 // If -cc1 came from a response file, remove the EOL sentinels.
 if (MarkEOLs) {
   auto newEnd = std::remove(argv.begin(), argv.end(), nullptr);
   argv.resize(newEnd - argv.begin());
 }
-return ExecuteCC1Tool(argv, argv[1] + 4);
+return ExecuteCC1Tool(argv, argv[1]);
   }
 
   bool CanonicalPrefixes = true;
Index: clang/tools/driver/CMakeLists.txt
===
--- clang/tools/driver/CMakeLists.txt
+++ clang/tools/driver/CMakeLists.txt
@@ -63,7 +63,7 @@
 add_dependencies(clang clang-resource-headers)
 
 if(NOT CLANG_LINKS_TO_CREATE)
-  set(CLANG_LINKS_TO_CREATE clang++ clang-cl clang-cpp)
+  set(CLANG_LINKS_TO_CREATE clang++ clang-cl clang-cpp flang)
 endif()
 
 foreach(link ${CLANG_LINKS_TO_CREATE})
Index: clang/lib/Driver/Types.cpp
===
--- clang/lib/Driver/Types.cpp
+++ clang/lib/Driver/Types.cpp
@@ -187,6 +187,16 @@
   }
 }
 
+bool types::isFortran(ID Id) {
+  switch (Id) {
+  default:
+return false;
+
+  case TY_Fortran: case TY_PP_Fortran:
+return true;
+  }
+}
+
 bool types::isSrcFile(ID Id) {
   return Id != TY_Object && getPreprocessedType(Id) != TY_INVALID;
 }