[PATCH] D83550: [PATCH 1/4][Sema][AArch64] Add parsing support for arm_sve_vector_bits attribute

[Sander de Smalen via Phabricator via cfe-commits]

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

Thanks for the changes @c-rhodes, the use of a LANGOPT for this makes sense to me.
With my nits addressed, I'm happy with the patch if @aaron.ballman is.



================
Comment at: clang/include/clang/Basic/AttrDocs.td:4861
+  let Content = [{
+The ``arm_sve_vector_bits(N)`` attribute is defined by the Arm C Language
+Extensions (ACLE) for SVE. It is used to define fixed-length (VLST) variants of
----------------
nit: Can you add a line here to state this feature is still WIP ? (we can remove that in a later patch)
I know this is implied by your comment re __ARM_FEATURE_SVE_BITS being defined, but I guess it's better to be explicit to avoid confusion.


================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:381
+  if (Opts.ArmSveVectorBits)
+    Builder.defineMacro("__ARM_FEATURE_SVE_BITS__EXPERIMENTAL",
+                        Twine(Opts.ArmSveVectorBits));
----------------
nit: `s/__ARM_FEATURE_SVE_BITS__EXPERIMENTAL/__ARM_FEATURE_SVE_BITS_EXPERIMENTAL/` ?


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1723
+    const Driver &D = getToolChain().getDriver();
+    if (!Val.equals("128") && !Val.equals("256") && !Val.equals("512") &&
+        !Val.equals("1024") && !Val.equals("2048")) {
----------------
[feel free to ignore] I don't think it necessarily needs to be restricted to this set, it should be able to support any multiple of 128bits. Although if you want to limit it for the first implementation that's fine.


================
Comment at: clang/lib/Sema/SemaType.cpp:7733
   }
+
   // The number of elements must be an ICE.
----------------
nit: unrelated change.


================
Comment at: clang/lib/Sema/SemaType.cpp:7789
+  llvm::APSInt SveVectorSizeInBits(32);
+  if (!validIntegerConstantExpr(S, Attr, SveVectorSizeInBits))
+    return;
----------------
nit: can you rename this function to something like `verifyValidIntegerConstantExpr` to make it clear that that function gives a diagnostic if it fails?


================
Comment at: clang/test/Sema/attr-arm-sve-vector-bits.c:1
+// REQUIRES: aarch64-registered-target
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -fsyntax-only -verify -msve-vector-bits=128 -fallow-half-arguments-and-returns %s
----------------
nit: Given that this test uses `-fsyntax-only`, can you do the same as you did for `clang/test/Driver/aarch64-sve-vector-bits.c` and remove `aarch64-registered-target` and `#include <arm_sve.h>` and do the tests on the builtin types instead? e.g.

```typedef __SVInt32_t svint32_t;
typedef svint32_t fixed_int32_t __attribute__((arm_sve_vector_bits(N)));```


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

https://reviews.llvm.org/D83550