Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Clang] Remove preprocessor guards and global feature checks for NEON #95102

Closed
wants to merge 145 commits into from

Conversation

Lukacma
Copy link
Contributor

@Lukacma Lukacma commented Jun 11, 2024

To enable function multi-versioning (FMV), current checks which rely on cmd line options or global macros to see if target feature is present need to be removed. This patch removes those for NEON and also implements changes to NEON header file as proposed in ACLE.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 11, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jun 11, 2024

@llvm/pr-subscribers-clang

Author: None (Lukacma)

Changes

To enable function multi-versioning (FMV), current checks which rely on cmd line options or global macros to see if target feature is present need to be removed. This patch removes those for NEON and also implements changes to NEON header file as proposed in ACLE.


Full diff: https://github.com/llvm/llvm-project/pull/95102.diff

4 Files Affected:

  • (modified) clang/lib/Sema/SemaType.cpp (-23)
  • (modified) clang/test/Sema/arm-vector-types-support.c (-2)
  • (removed) clang/test/SemaCUDA/neon-attrs.cu (-22)
  • (modified) clang/utils/TableGen/NeonEmitter.cpp (-5)
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 441fdcca0758f..65b87f62e294f 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -8084,29 +8084,6 @@ static void HandleNeonVectorTypeAttr(QualType &CurType, const ParsedAttr &Attr,
         AuxTI && (AuxTI->getTriple().isAArch64() || AuxTI->getTriple().isARM());
   }
 
-  // Target must have NEON (or MVE, whose vectors are similar enough
-  // not to need a separate attribute)
-  if (!(S.Context.getTargetInfo().hasFeature("neon") ||
-        S.Context.getTargetInfo().hasFeature("mve") ||
-        S.Context.getTargetInfo().hasFeature("sve") ||
-        S.Context.getTargetInfo().hasFeature("sme") ||
-        IsTargetCUDAAndHostARM) &&
-      VecKind == VectorKind::Neon) {
-    S.Diag(Attr.getLoc(), diag::err_attribute_unsupported)
-        << Attr << "'neon', 'mve', 'sve' or 'sme'";
-    Attr.setInvalid();
-    return;
-  }
-  if (!(S.Context.getTargetInfo().hasFeature("neon") ||
-        S.Context.getTargetInfo().hasFeature("mve") ||
-        IsTargetCUDAAndHostARM) &&
-      VecKind == VectorKind::NeonPoly) {
-    S.Diag(Attr.getLoc(), diag::err_attribute_unsupported)
-        << Attr << "'neon' or 'mve'";
-    Attr.setInvalid();
-    return;
-  }
-
   // Check the attribute arguments.
   if (Attr.getNumArgs() != 1) {
     S.Diag(Attr.getLoc(), diag::err_attribute_wrong_number_arguments)
diff --git a/clang/test/Sema/arm-vector-types-support.c b/clang/test/Sema/arm-vector-types-support.c
index ed5f5ba175a94..1d2e1c9336fc6 100644
--- a/clang/test/Sema/arm-vector-types-support.c
+++ b/clang/test/Sema/arm-vector-types-support.c
@@ -2,6 +2,4 @@
 // RUN: %clang_cc1 %s -triple aarch64 -fsyntax-only -verify
 // RUN: %clang_cc1 %s -triple aarch64 -target-feature -fp-armv8 -target-abi aapcs-soft -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', 'mve', 'sve' or 'sme'; 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=}}
diff --git a/clang/test/SemaCUDA/neon-attrs.cu b/clang/test/SemaCUDA/neon-attrs.cu
deleted file mode 100644
index 129056741ac9a..0000000000000
--- a/clang/test/SemaCUDA/neon-attrs.cu
+++ /dev/null
@@ -1,22 +0,0 @@
-// CPU-side compilation on ARM with neon enabled (no errors expected).
-// RUN: %clang_cc1 -triple arm64-linux-gnu -target-feature +neon -aux-triple nvptx64 -x cuda -fsyntax-only -verify=quiet %s
-
-// CPU-side compilation on ARM with neon disabled.
-// RUN: %clang_cc1 -triple arm64-linux-gnu -target-feature -neon -aux-triple nvptx64 -x cuda -fsyntax-only -verify %s
-
-// GPU-side compilation on ARM (no errors expected).
-// RUN: %clang_cc1 -triple nvptx64 -aux-triple arm64-linux-gnu -fcuda-is-device -x cuda -fsyntax-only -verify=quiet %s
-
-// Regular C++ compilation on ARM with neon enabled (no errors expected).
-// RUN: %clang_cc1 -triple arm64-linux-gnu -target-feature +neon -x c++ -fsyntax-only -verify=quiet %s
-
-// Regular C++ compilation on ARM with neon disabled.
-// RUN: %clang_cc1 -triple arm64-linux-gnu -target-feature -neon -x c++ -fsyntax-only -verify %s
-
-// quiet-no-diagnostics
-typedef __attribute__((neon_vector_type(4))) float float32x4_t;
-// expected-error@-1 {{'neon_vector_type' attribute is not supported on targets missing 'neon', 'mve', 'sve' or 'sme'}}
-// expect
-typedef unsigned char poly8_t;
-typedef __attribute__((neon_polyvector_type(8))) poly8_t poly8x8_t;
-// expected-error@-1 {{'neon_polyvector_type' attribute is not supported on targets missing 'neon' or 'mve'}}
diff --git a/clang/utils/TableGen/NeonEmitter.cpp b/clang/utils/TableGen/NeonEmitter.cpp
index 56f1fdf9ef574..626031d38cf00 100644
--- a/clang/utils/TableGen/NeonEmitter.cpp
+++ b/clang/utils/TableGen/NeonEmitter.cpp
@@ -2370,10 +2370,6 @@ void NeonEmitter::run(raw_ostream &OS) {
         "Please use -mfloat-abi=softfp or -mfloat-abi=hard\"\n";
   OS << "#else\n\n";
 
-  OS << "#if !defined(__ARM_NEON)\n";
-  OS << "#error \"NEON support not enabled\"\n";
-  OS << "#else\n\n";
-
   OS << "#include <stdint.h>\n\n";
 
   OS << "#include <arm_bf16.h>\n";
@@ -2450,7 +2446,6 @@ void NeonEmitter::run(raw_ostream &OS) {
   OS << "#undef __ai\n\n";
   OS << "#endif /* if !defined(__ARM_NEON) */\n";
   OS << "#endif /* ifndef __ARM_FP */\n";
-  OS << "#endif /* __ARM_NEON_H */\n";
 }
 
 /// run - Read the records in arm_fp16.td and output arm_fp16.h.  arm_fp16.h

Attr.setInvalid();
return;
}
if (!(S.Context.getTargetInfo().hasFeature("neon") ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we may need to leave still the test for MVE. We need to leave a comment in the ACLE stating that the MVE header is still needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we ? From my understanding this checks are Neon vector types and are unrelated to MVE. The only reason MVE is used is because MVE vectors are similar enough so we can use them as neon vectors

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can preserve the behaviour for MVE if you alter the diagnostics condition to be
"NEON type seen" && "no MVE" && "compiling for M-class".

e-kwsm and others added 22 commits June 12, 2024 11:17
Introduced by 2cf1439 (llvm#94279).
See also 6c369cf.
The build system cannot track transitive dependencies on generated
headers for some reason.
…m#94907)

After enhancing MSVC's STL to statically initialize our condition_variable,
Clang began noticing that some mutexes in the test suite were unused.
nvcc warns about the following code:

`void f();
__device__ void f() {}`

but clang does not since clang allows device function to overload host
function.

Users want clang to emit similar warning to help code to be compatible
with nvcc.

Since this may cause regression with existing code, the warning is off
by default and can be enabled by -Wnvcc-compat.

It won't cause warning in system headers, even with -Wnvcc-compat.
…ly (llvm#94212)

The patch didn't consistently clean up `#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS` and '#if defined(LLVM_ENABLE_ABI_BREAKING_CHECKS)' paths, causing a lot of build failures
Otherwise these tests would fail when using gnuwin32.
…#94982)

Reverts llvm#94212
Some codes assume that `NDEBUG` implies
`LLVM_ENABLE_ABI_BREAKING_CHECKS`, thus llvm#94212 breaks some build bots.
…updates (llvm#91725)

Related review: llvm#91724

This patch updates the RemoveDIs migration document to include details
on the textual IR changes, including steps to update any downstream lit
tests accordingly. These steps are the same as those used to update the
lit tests in the LLVM/Clang lit tests, as detailed in the review linked
above.
This was probably intended to test the immediate offset addressing
mode. Add some tests to check immediate offsets, and clean up run lines.
Since inreg now works for the default calling convention, we can
check the output with fewer argument shuffling instructions.
We can't use `dladdr()` in the tests, because when we're statically
linking with musl that function is a no-op.

Additionally, because musl disables emission of unwind information in
its build, and because its signal trampolines don't include unwind
information, tests that involve unwinding through a signal handler won't
work and need to be disabled for musl.

rdar://123436891
Just use the original type and let it hit a standard legalization error.
…vm#94931)

The test clang/test/OpenMP/error_unsupport_feature.c don't check the
output written to the current directory. The current directory may be
write protected e.g. in a sandboxed environment.

This patch replace the -emit-llvm option with -emit-llvm-only as it
don't care about the outputed llvm IR.
… of Linux targets (llvm#94672)

The different build configuration and target Linux system can load a
different number of .so libraries. Add and check own libraries.
…4712)

When the integer range analysis was first develop, a pass that did
integer range-based constant folding was developed and used as a test
pass. There was an intent to add such a folding to SCCP, but that hasn't
happened.

Meanwhile, -int-range-optimizations was added to the arith dialect's
transformations. The cmpi simplification in that pass is a strict subset
of the constant folding that lived in
-test-int-range-inference.

This commit moves the former test pass into -int-range-optimizaitons,
subsuming its previous contents. It also adds an optimization from
rocMLIR where `rem{s,u}i` operations that are noops are replaced by
their left operands.
Static verifier reports unchecked use of pointer after explicitly
checking earlier in the function. It appears the pointer won't be a
nullptr, so remove the unneeded check for consistency.
NagyDonat and others added 6 commits June 12, 2024 11:18
…ession (llvm#94356)

This commit reimplements the functionality of the Clang Static Analyzer
checker `alpha.core.SizeofPointer` within clang-tidy by adding a new
(off-by-default) option to bugprone-sizeof-expression which activates
reporting all the `sizeof(ptr)` expressions (where ptr is an expression
that produces a pointer).

The main motivation for this change is that `alpha.core.SizeofPointer`
was an AST-based checker, which did not rely on the path sensitive
capabilities of the Static Analyzer, so there was no reason to keep it
in the Static Analyzer instead of the more lightweight clang-tidy.

After this commit I'm planning to create a separate commit that deletes
`alpha.core.SizeofPointer` from Clang Static Analyzer.

It was natural to place this moved logic in bugprone-sizeof-expression,
because that check already provided several heuristics that reported
various especially suspicious classes of `sizeof(ptr)` expressions.

The new mode `WarnOnSizeOfPointer` is off-by-default, so it won't
surprise the existing users; but it can provide a more through coverage
for the vulnerability CWE-467 ("Use of sizeof() on a Pointer Type") than
the existing partial heuristics.

Previously this checker had an exception that the RHS of a
`sizeof(array) / sizeof(array[0])` expression is not reported; I
generalized this to an exception that the check doesn't report
`sizeof(expr[0])` and `sizeof(*expr)`. This idea is taken from the
Static Analyzer checker `alpha.core.SizeofPointer` (which had an
exception for `*expr`), but analysis of open source projects confirmed
that this indeed eliminates lots of unwanted results.

Note that the suppression of `sizeof(expr[0])` and `sizeof(*expr)`
reports also affects the "old" mode `WarnOnSizeOfPointerToAggregate`
which is enabled by default.

This commit also replaces the old message "suspicious usage of
'sizeof(A*)'; pointer to aggregate" with two more concrete messages; but
I feel that this tidy check would deserve a through cleanup of all the
diagnostic messages that it can produce. (I added a FIXME to mark one
outright misleading message.)
Those BitVectors get expensive on targets like AMDGPU with thousands of
registers, and RegAliasIterator is also expensive.

We can move all liveness calculations to use RegUnits instead to speed
it up for targets where RegAliasIterator is expensive, like AMDGPU.
On targets where RegAliasIterator is cheap, this alternative can be a little more expensive, but I believe the tradeoff is worth it.
This PR is required to fix
`std/algorithms/alg.nonmodifying/mismatch/mismatch.pass.cpp` test for
big endian platrofrms such as z/OS.
…anslation (llvm#95098)"

Reverted due to failure on buildbot due to missing use of the
WriteNewDbgInfoFormat flag in MLIR.

This reverts commit ca920bb.
Add a section about fence & address spaces that covers amdgpu-as.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.