[PATCH] D83553: [PATCH 3/4][Sema][AArch64] Add codegen for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes abandoned this revision.
c-rhodes added a comment.

I've posted a prototype D85128  with an 
alternative implementation, given it's quite different to this patch I've 
posted it as a separate patch and am abandoning this one. See new patch for 
more details, cheers


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

https://reviews.llvm.org/D83553

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

[PATCH] D83553: [PATCH 3/4][Sema][AArch64] Add codegen for arm_sve_vector_bits attribute

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:152
+  Align, Name,
+  /*ArraySize=*/nullptr, Alloca);
 

c-rhodes wrote:
> efriedma wrote:
> > Do we need to bitcast the result of CreateTempAlloca to a pointer to the 
> > array type?  I'm concerned that we might miss a bitcast if the source code 
> > uses the address of the variable.
> > Do we need to bitcast the result of CreateTempAlloca to a pointer to the 
> > array type? I'm concerned that we might miss a bitcast if the source code 
> > uses the address of the variable.
> 
> You were right, I've spent some time investigating this. The current 
> implementation crashes on:
> ```fixed_int32_t global;
> fixed_int32_t address_of_global() {
>   fixed_int32_t *global_ptr;
>   global_ptr = 
>   return *global_ptr;
> }```
> 
> the reason being `global` is represented as an `ArrayType` whereas the 
> pointer `global_ptr` is scalable:
> 
> ```@global = global [4 x i32] zeroinitializer, align 16
> %global_ptr = alloca *, align 8```
> 
> so when storing the address of `global` to `global_ptr` the store it tries to 
> create causes a crash:
> 
> `store [4 x i32]* @global, ** %global_ptr, align 8`
> 
> I tried your suggestion to bitcast to alloca to the array type in 
> `CreateMemTemp` but found for that example it isn't called, it's created by a 
> call to `CreateTempAlloca` in CGDecl.cpp (`EmitAutoVarAlloca`). 
> `CreateTempAlloca` takes an `llvm::Type *Ty` so it's not as straightforward 
> as doing a bitcast there, although I found it could be done in 
> `EmitAutoVarAlloca` but it means having to handle this is two places I'm 
> aware of and potentially others I haven't hit. In this case as well it also 
> required looking through the pointer to see if the pointee was a VLST then 
> doing a bitcast.
> 
> I've also experimented with representing allocas as fixed-length arrays to 
> see if that will make it any easier and it does simplify the patch a little. 
> It does require handling `PointerType` in `ConvertTypeForMem` however as we 
> do for `ConstantArray`, same issue I mentioned in response to your other 
> comment about removing that.
> 
> I planning to update the patch with that implementation but I've just found 
> another issue:
> 
> ```fixed_int32_t arr[3];
> fixed_int32_t *z() {
>   fixed_int32_t *array_ptr;
>   array_ptr = [0];
>   return array_ptr;
> }```
> 
> trying to create a store:
> `store [4 x i32]* %0, ** %retval, align 8`
> 
> although this is done in CGStmt.cpp as it's for a retval so it looks like a 
> bitcast could also be required there.
I think a `fixed_int32_t *` needs to be converted to `[4 x i32]*`, for the sake 
of consistency... but see also my other comment.



Comment at: clang/lib/CodeGen/CodeGenTypes.cpp:151
+  return llvm::ArrayType::get(*MemTy, A->getSize().getZExtValue());
+  }
+

c-rhodes wrote:
> efriedma wrote:
> > I think the default handling for constant arrays should do the right thing, 
> > now that we've changed the default behavior of ConvertTypeForMem.
> > I think the default handling for constant arrays should do the right thing, 
> > now that we've changed the default behavior of ConvertTypeForMem.
> 
> `ConvertType` looks at the canonical type so the type attribute is lost.
That sounds like a bug in the AST: since isVLST() affects the semantics of the 
type, it needs to be part of the canonical type. Otherwise you're going to be 
finding bugs all over in both Sema and CodeGen.


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

https://reviews.llvm.org/D83553



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

[PATCH] D83553: [PATCH 3/4][Sema][AArch64] Add codegen for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes marked an inline comment as done.
c-rhodes added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:152
+  Align, Name,
+  /*ArraySize=*/nullptr, Alloca);
 

efriedma wrote:
> Do we need to bitcast the result of CreateTempAlloca to a pointer to the 
> array type?  I'm concerned that we might miss a bitcast if the source code 
> uses the address of the variable.
> Do we need to bitcast the result of CreateTempAlloca to a pointer to the 
> array type? I'm concerned that we might miss a bitcast if the source code 
> uses the address of the variable.

You were right, I've spent some time investigating this. The current 
implementation crashes on:
```fixed_int32_t global;
fixed_int32_t address_of_global() {
  fixed_int32_t *global_ptr;
  global_ptr = 
  return *global_ptr;
}```

the reason being `global` is represented as an `ArrayType` whereas the pointer 
`global_ptr` is scalable:

```@global = global [4 x i32] zeroinitializer, align 16
%global_ptr = alloca *, align 8```

so when storing the address of `global` to `global_ptr` the store it tries to 
create causes a crash:

`store [4 x i32]* @global, ** %global_ptr, align 8`

I tried your suggestion to bitcast to alloca to the array type in 
`CreateMemTemp` but found for that example it isn't called, it's created by a 
call to `CreateTempAlloca` in CGDecl.cpp (`EmitAutoVarAlloca`). 
`CreateTempAlloca` takes an `llvm::Type *Ty` so it's not as straightforward as 
doing a bitcast there, although I found it could be done in `EmitAutoVarAlloca` 
but it means having to handle this is two places I'm aware of and potentially 
others I haven't hit. In this case as well it also required looking through the 
pointer to see if the pointee was a VLST then doing a bitcast.

I've also experimented with representing allocas as fixed-length arrays to see 
if that will make it any easier and it does simplify the patch a little. It 
does require handling `PointerType` in `ConvertTypeForMem` however as we do for 
`ConstantArray`, same issue I mentioned in response to your other comment about 
removing that.

I planning to update the patch with that implementation but I've just found 
another issue:

```fixed_int32_t arr[3];
fixed_int32_t *z() {
  fixed_int32_t *array_ptr;
  array_ptr = [0];
  return array_ptr;
}```

trying to create a store:
`store [4 x i32]* %0, ** %retval, align 8`

although this is done in CGStmt.cpp as it's for a retval so it looks like a 
bitcast could also be required there.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3985
+else
+  Init = EmitNullConstant(D->getType());
   } else {

efriedma wrote:
> EmitNullConstant should just do the right thing, I think, now that we've 
> changed the default behavior of ConvertTypeForMem.
> EmitNullConstant should just do the right thing, I think, now that we've 
> changed the default behavior of ConvertTypeForMem.

Good spot, these changes can be removed



Comment at: clang/lib/CodeGen/CodeGenTypes.cpp:151
+  return llvm::ArrayType::get(*MemTy, A->getSize().getZExtValue());
+  }
+

efriedma wrote:
> I think the default handling for constant arrays should do the right thing, 
> now that we've changed the default behavior of ConvertTypeForMem.
> I think the default handling for constant arrays should do the right thing, 
> now that we've changed the default behavior of ConvertTypeForMem.

`ConvertType` looks at the canonical type so the type attribute is lost.


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

https://reviews.llvm.org/D83553



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

[PATCH] D83553: [PATCH 3/4][Sema][AArch64] Add codegen for arm_sve_vector_bits attribute

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:152
+  Align, Name,
+  /*ArraySize=*/nullptr, Alloca);
 

Do we need to bitcast the result of CreateTempAlloca to a pointer to the array 
type?  I'm concerned that we might miss a bitcast if the source code uses the 
address of the variable.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3985
+else
+  Init = EmitNullConstant(D->getType());
   } else {

EmitNullConstant should just do the right thing, I think, now that we've 
changed the default behavior of ConvertTypeForMem.



Comment at: clang/lib/CodeGen/CodeGenTypes.cpp:151
+  return llvm::ArrayType::get(*MemTy, A->getSize().getZExtValue());
+  }
+

I think the default handling for constant arrays should do the right thing, now 
that we've changed the default behavior of ConvertTypeForMem.


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

https://reviews.llvm.org/D83553



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

[PATCH] D83553: [PATCH 3/4][Sema][AArch64] Add codegen for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes marked an inline comment as done.
c-rhodes added inline comments.



Comment at: clang/lib/CodeGen/CodeGenTypes.h:138
+  llvm::Type *ConvertTypeForMem(QualType T, bool ForBitField = false,
+bool EnforceFixedLengthSVEAttribute = false);
 

efriedma wrote:
> c-rhodes wrote:
> > efriedma wrote:
> > > The default for EnforceFixedLengthSVEAttribute seems backwards; I would 
> > > expect that almost everywhere that calls ConvertTypeForMem actually wants 
> > > the fixed-length type.  The scalable type only exists in registers.
> > > The default for EnforceFixedLengthSVEAttribute seems backwards; I would 
> > > expect that almost everywhere that calls ConvertTypeForMem actually wants 
> > > the fixed-length type. The scalable type only exists in registers.
> > 
> > It has no effect unless `T->isVLST()` so I think it makes sense.
> My question is "why is the current default for EnforceFixedLengthSVEAttribute 
> correct?" You answer for that is "because VLST types are rare"?  I'm not sure 
> how that's related.
> 
> Essentially, the issue is that ConvertTypeForMem means "I'm allocating 
> something in memory; what is its type?".  Except for a few places where we've 
> specifically added handling to make it work, the code assumes scalable types 
> don't exist.  So in most places, we want the fixed version.  With the current 
> default, I'm afraid we're going to end up with weird failures with various 
> constructs you haven't tested.
> 
> I guess if there's some large number of places where the current default is 
> actually beneficial, the current patch wouldn't make it obvious, but my 
> intuition is that are few places like that.
>> My question is "why is the current default for 
>> EnforceFixedLengthSVEAttribute correct?" You answer for that is "because 
>> VLST types are rare"? I'm not sure how that's related.

>Essentially, the issue is that ConvertTypeForMem means "I'm allocating 
>something in memory; what is its type?". Except for a few places where we've 
>specifically added handling to make it work, the code assumes scalable types 
>don't exist. So in most places, we want the fixed version. With the current 
>default, I'm afraid we're going to end up with weird failures with various 
>constructs you haven't tested.

Sorry I misunderstood what you meant. I think you're right that does make 
sense, I guess the benefit of defaulting to false is (hopefully) those failures 
would have come to our attention and we could explicitly add test cases for 
those, although I suspect the same applies with your suggestion with the added 
benefit of us supporting constructs we haven't explicitly tested as you say. 
Anyhow, I've made the change, cheers!


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

https://reviews.llvm.org/D83553



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

[PATCH] D83553: [PATCH 3/4][Sema][AArch64] Add codegen for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes updated this revision to Diff 279265.
c-rhodes added a comment.

Change the default for `EnforceFixedLengthSVEAttribute`.


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

https://reviews.llvm.org/D83553

Files:
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenTypes.cpp
  clang/lib/CodeGen/CodeGenTypes.h
  clang/test/Sema/attr-arm-sve-vector-bits-bitcast.c
  clang/test/Sema/attr-arm-sve-vector-bits-call.c
  clang/test/Sema/attr-arm-sve-vector-bits-cast.c
  clang/test/Sema/attr-arm-sve-vector-bits-codegen.c
  clang/test/Sema/attr-arm-sve-vector-bits-globals.c
  clang/test/Sema/attr-arm-sve-vector-bits-types.c

Index: clang/test/Sema/attr-arm-sve-vector-bits-types.c
===
--- /dev/null
+++ clang/test/Sema/attr-arm-sve-vector-bits-types.c
@@ -0,0 +1,525 @@
+// REQUIRES: aarch64-registered-target
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -msve-vector-bits=128 -fallow-half-arguments-and-returns -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-128
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -msve-vector-bits=256 -fallow-half-arguments-and-returns -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-256
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -msve-vector-bits=512 -fallow-half-arguments-and-returns -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-512
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -msve-vector-bits=1024 -fallow-half-arguments-and-returns -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-1024
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -msve-vector-bits=2048 -fallow-half-arguments-and-returns -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-2048
+
+#include 
+
+#define N __ARM_FEATURE_SVE_BITS_EXPERIMENTAL
+
+typedef svint8_t fixed_int8_t __attribute__((arm_sve_vector_bits(N)));
+typedef svint16_t fixed_int16_t __attribute__((arm_sve_vector_bits(N)));
+typedef svint32_t fixed_int32_t __attribute__((arm_sve_vector_bits(N)));
+typedef svint64_t fixed_int64_t __attribute__((arm_sve_vector_bits(N)));
+
+typedef svuint8_t fixed_uint8_t __attribute__((arm_sve_vector_bits(N)));
+typedef svuint16_t fixed_uint16_t __attribute__((arm_sve_vector_bits(N)));
+typedef svuint32_t fixed_uint32_t __attribute__((arm_sve_vector_bits(N)));
+typedef svuint64_t fixed_uint64_t __attribute__((arm_sve_vector_bits(N)));
+
+typedef svfloat16_t fixed_float16_t __attribute__((arm_sve_vector_bits(N)));
+typedef svfloat32_t fixed_float32_t __attribute__((arm_sve_vector_bits(N)));
+typedef svfloat64_t fixed_float64_t __attribute__((arm_sve_vector_bits(N)));
+
+typedef svbfloat16_t fixed_bfloat16_t __attribute__((arm_sve_vector_bits(N)));
+
+typedef svbool_t fixed_bool_t __attribute__((arm_sve_vector_bits(N)));
+
+//===--===//
+// Structs and unions
+//===--===//
+#define DEFINE_STRUCT(ty) \
+  struct struct_##ty {\
+fixed_##ty##_t x; \
+  } struct_##ty;
+
+#define DEFINE_UNION(ty) \
+  union union_##ty { \
+fixed_##ty##_t x;\
+  } union_##ty;
+
+DEFINE_STRUCT(int8)
+DEFINE_STRUCT(int16)
+DEFINE_STRUCT(int32)
+DEFINE_STRUCT(int64)
+DEFINE_STRUCT(uint8)
+DEFINE_STRUCT(uint16)
+DEFINE_STRUCT(uint32)
+DEFINE_STRUCT(uint64)
+DEFINE_STRUCT(float16)
+DEFINE_STRUCT(float32)
+DEFINE_STRUCT(float64)
+DEFINE_STRUCT(bfloat16)
+DEFINE_STRUCT(bool)
+
+DEFINE_UNION(int8)
+DEFINE_UNION(int16)
+DEFINE_UNION(int32)
+DEFINE_UNION(int64)
+DEFINE_UNION(uint8)
+DEFINE_UNION(uint16)
+DEFINE_UNION(uint32)
+DEFINE_UNION(uint64)
+DEFINE_UNION(float16)
+DEFINE_UNION(float32)
+DEFINE_UNION(float64)
+DEFINE_UNION(bfloat16)
+DEFINE_UNION(bool)
+
+//===--===//
+// Global variables
+//===--===//
+fixed_int8_t global_i8;
+fixed_int16_t global_i16;
+fixed_int32_t global_i32;
+fixed_int64_t global_i64;
+
+fixed_uint8_t global_u8;
+fixed_uint16_t global_u16;
+fixed_uint32_t global_u32;
+fixed_uint64_t global_u64;
+
+fixed_float16_t global_f16;
+fixed_float32_t global_f32;
+fixed_float64_t global_f64;
+
+fixed_bfloat16_t global_bf16;
+
+fixed_bool_t global_bool;
+
+//===--===//
+// Global arrays
+//===--===//
+fixed_int8_t global_arr_i8[3];
+fixed_int16_t global_arr_i16[3];
+fixed_int32_t global_arr_i32[3];
+fixed_int64_t global_arr_i64[3];
+

[PATCH] D83553: [PATCH 3/4][Sema][AArch64] Add codegen for arm_sve_vector_bits attribute

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CodeGenTypes.h:138
+  llvm::Type *ConvertTypeForMem(QualType T, bool ForBitField = false,
+bool EnforceFixedLengthSVEAttribute = false);
 

c-rhodes wrote:
> efriedma wrote:
> > The default for EnforceFixedLengthSVEAttribute seems backwards; I would 
> > expect that almost everywhere that calls ConvertTypeForMem actually wants 
> > the fixed-length type.  The scalable type only exists in registers.
> > The default for EnforceFixedLengthSVEAttribute seems backwards; I would 
> > expect that almost everywhere that calls ConvertTypeForMem actually wants 
> > the fixed-length type. The scalable type only exists in registers.
> 
> It has no effect unless `T->isVLST()` so I think it makes sense.
My question is "why is the current default for EnforceFixedLengthSVEAttribute 
correct?" You answer for that is "because VLST types are rare"?  I'm not sure 
how that's related.

Essentially, the issue is that ConvertTypeForMem means "I'm allocating 
something in memory; what is its type?".  Except for a few places where we've 
specifically added handling to make it work, the code assumes scalable types 
don't exist.  So in most places, we want the fixed version.  With the current 
default, I'm afraid we're going to end up with weird failures with various 
constructs you haven't tested.

I guess if there's some large number of places where the current default is 
actually beneficial, the current patch wouldn't make it obvious, but my 
intuition is that are few places like that.


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

https://reviews.llvm.org/D83553



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

[PATCH] D83553: [PATCH 3/4][Sema][AArch64] Add codegen for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes marked 5 inline comments as done.
c-rhodes added inline comments.



Comment at: clang/lib/CodeGen/CodeGenTypes.h:138
+  llvm::Type *ConvertTypeForMem(QualType T, bool ForBitField = false,
+bool EnforceFixedLengthSVEAttribute = false);
 

efriedma wrote:
> The default for EnforceFixedLengthSVEAttribute seems backwards; I would 
> expect that almost everywhere that calls ConvertTypeForMem actually wants the 
> fixed-length type.  The scalable type only exists in registers.
> The default for EnforceFixedLengthSVEAttribute seems backwards; I would 
> expect that almost everywhere that calls ConvertTypeForMem actually wants the 
> fixed-length type. The scalable type only exists in registers.

It has no effect unless `T->isVLST()` so I think it makes sense.


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

https://reviews.llvm.org/D83553



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

[PATCH] D83553: [PATCH 3/4][Sema][AArch64] Add codegen for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes updated this revision to Diff 278466.
c-rhodes added a comment.

Changes:

- Rebased.
- Added comments for args in calls to `ConvertTypeForMem` when 
`EnforceFixedLengthSVEAttribute` is set and documented 
`EnforceFixedLengthSVEAttribute`.
- `s/getFixedSVETypeForMemory/getFixedLengthSVETypeForMemory/`
- Documented memory representation for fixed-length predicates.


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

https://reviews.llvm.org/D83553

Files:
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenTypes.cpp
  clang/lib/CodeGen/CodeGenTypes.h
  clang/test/Sema/attr-arm-sve-vector-bits-bitcast.c
  clang/test/Sema/attr-arm-sve-vector-bits-call.c
  clang/test/Sema/attr-arm-sve-vector-bits-cast.c
  clang/test/Sema/attr-arm-sve-vector-bits-codegen.c
  clang/test/Sema/attr-arm-sve-vector-bits-globals.c
  clang/test/Sema/attr-arm-sve-vector-bits-types.c

Index: clang/test/Sema/attr-arm-sve-vector-bits-types.c
===
--- /dev/null
+++ clang/test/Sema/attr-arm-sve-vector-bits-types.c
@@ -0,0 +1,525 @@
+// REQUIRES: aarch64-registered-target
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -msve-vector-bits=128 -fallow-half-arguments-and-returns -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-128
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -msve-vector-bits=256 -fallow-half-arguments-and-returns -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-256
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -msve-vector-bits=512 -fallow-half-arguments-and-returns -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-512
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -msve-vector-bits=1024 -fallow-half-arguments-and-returns -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-1024
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -msve-vector-bits=2048 -fallow-half-arguments-and-returns -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-2048
+
+#include 
+
+#define N __ARM_FEATURE_SVE_BITS_EXPERIMENTAL
+
+typedef svint8_t fixed_int8_t __attribute__((arm_sve_vector_bits(N)));
+typedef svint16_t fixed_int16_t __attribute__((arm_sve_vector_bits(N)));
+typedef svint32_t fixed_int32_t __attribute__((arm_sve_vector_bits(N)));
+typedef svint64_t fixed_int64_t __attribute__((arm_sve_vector_bits(N)));
+
+typedef svuint8_t fixed_uint8_t __attribute__((arm_sve_vector_bits(N)));
+typedef svuint16_t fixed_uint16_t __attribute__((arm_sve_vector_bits(N)));
+typedef svuint32_t fixed_uint32_t __attribute__((arm_sve_vector_bits(N)));
+typedef svuint64_t fixed_uint64_t __attribute__((arm_sve_vector_bits(N)));
+
+typedef svfloat16_t fixed_float16_t __attribute__((arm_sve_vector_bits(N)));
+typedef svfloat32_t fixed_float32_t __attribute__((arm_sve_vector_bits(N)));
+typedef svfloat64_t fixed_float64_t __attribute__((arm_sve_vector_bits(N)));
+
+typedef svbfloat16_t fixed_bfloat16_t __attribute__((arm_sve_vector_bits(N)));
+
+typedef svbool_t fixed_bool_t __attribute__((arm_sve_vector_bits(N)));
+
+//===--===//
+// Structs and unions
+//===--===//
+#define DEFINE_STRUCT(ty) \
+  struct struct_##ty {\
+fixed_##ty##_t x; \
+  } struct_##ty;
+
+#define DEFINE_UNION(ty) \
+  union union_##ty { \
+fixed_##ty##_t x;\
+  } union_##ty;
+
+DEFINE_STRUCT(int8)
+DEFINE_STRUCT(int16)
+DEFINE_STRUCT(int32)
+DEFINE_STRUCT(int64)
+DEFINE_STRUCT(uint8)
+DEFINE_STRUCT(uint16)
+DEFINE_STRUCT(uint32)
+DEFINE_STRUCT(uint64)
+DEFINE_STRUCT(float16)
+DEFINE_STRUCT(float32)
+DEFINE_STRUCT(float64)
+DEFINE_STRUCT(bfloat16)
+DEFINE_STRUCT(bool)
+
+DEFINE_UNION(int8)
+DEFINE_UNION(int16)
+DEFINE_UNION(int32)
+DEFINE_UNION(int64)
+DEFINE_UNION(uint8)
+DEFINE_UNION(uint16)
+DEFINE_UNION(uint32)
+DEFINE_UNION(uint64)
+DEFINE_UNION(float16)
+DEFINE_UNION(float32)
+DEFINE_UNION(float64)
+DEFINE_UNION(bfloat16)
+DEFINE_UNION(bool)
+
+//===--===//
+// Global variables
+//===--===//
+fixed_int8_t global_i8;
+fixed_int16_t global_i16;
+fixed_int32_t global_i32;
+fixed_int64_t global_i64;
+
+fixed_uint8_t global_u8;
+fixed_uint16_t global_u16;
+fixed_uint32_t global_u32;
+fixed_uint64_t global_u64;
+
+fixed_float16_t global_f16;
+fixed_float32_t global_f32;
+fixed_float64_t global_f64;
+
+fixed_bfloat16_t global_bf16;
+
+fixed_bool_t global_bool;
+
+//===--===//
+// Global arrays

[PATCH] D83553: [PATCH 3/4][Sema][AArch64] Add codegen for arm_sve_vector_bits attribute

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

In D83553#2151591 , @sdesmalen wrote:

> In D83553#2148429 , @efriedma wrote:
>
> > > If you mean alloca's for single vectors
> >
> > I was really referring to the IR values themselves, not the memory 
> > representation.  Since the width of the vectors is known, you could emit IR 
> > without any mention of scalable types at all (assuming the backend was 
> > extended to handle the intrinsics).
>
>
> That's right, the reason is because codegen of the intrinsics currently only 
> works on scalable types. By casting the pointer to a vscale-pointer, all IR 
> values are always scalable so we don't need to worry about doing things like 
> reinterpet_cast from a scalable to fixed-width vector, or vice versa.


I guess that's reasonable.  I suspect we're eventually going to end up with 
that functionality anyway, but maybe not right now.




Comment at: clang/lib/CodeGen/CodeGenTypes.h:138
+  llvm::Type *ConvertTypeForMem(QualType T, bool ForBitField = false,
+bool EnforceFixedLengthSVEAttribute = false);
 

The default for EnforceFixedLengthSVEAttribute seems backwards; I would expect 
that almost everywhere that calls ConvertTypeForMem actually wants the 
fixed-length type.  The scalable type only exists in registers.


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

https://reviews.llvm.org/D83553



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

[PATCH] D83553: [PATCH 3/4][Sema][AArch64] Add codegen for arm_sve_vector_bits attribute

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen added a comment.

In D83553#2148429 , @efriedma wrote:

> > If you mean alloca's for single vectors
>
> I was really referring to the IR values themselves, not the memory 
> representation.  Since the width of the vectors is known, you could emit IR 
> without any mention of scalable types at all (assuming the backend was 
> extended to handle the intrinsics).


That's right, the reason is because codegen of the intrinsics currently only 
works on scalable types. By casting the pointer to a vscale-pointer, all IR 
values are always scalable so we don't need to worry about doing things like 
reinterpet_cast from a scalable to fixed-width vector, or vice versa.


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

https://reviews.llvm.org/D83553



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

[PATCH] D83553: [PATCH 3/4][Sema][AArch64] Add codegen for arm_sve_vector_bits attribute

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> If you mean alloca's for single vectors

I was really referring to the IR values themselves, not the memory 
representation.  Since the width of the vectors is known, you could emit IR 
without any mention of scalable types at all (assuming the backend was extended 
to handle the intrinsics).

The choice of vscale'ed types for variables is also interesting, though. Thanks 
for the explanation.


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

https://reviews.llvm.org/D83553



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

[PATCH] D83553: [PATCH 3/4][Sema][AArch64] Add codegen for arm_sve_vector_bits attribute

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen added a comment.

In D83553#2145227 , @efriedma wrote:

> What's the tradeoff of representing these in IR as vscale'ed vector types, as 
> opposed to fixed-wdith vector types?


If you mean alloca's for single vectors, then that's partly to do with better 
test coverage of the stackframe layout with scalable vectors until we can start 
testing that with auto-vectorized code. Also, currently LLVM only implements 
the VL-scaled addressing modes for the scalable IR type and would otherwise 
always use base addressing mode if the type is fixed-width (`basereg = sp/fp + 
byteoffset; ld1 dstreg, [basereg, #0 mul VL]`), so until we add those smarts, 
code quality will probably be better.




Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:135
   llvm::Type *getStorageType(const FieldDecl *FD) {
-llvm::Type *Type = Types.ConvertTypeForMem(FD->getType());
+llvm::Type *Type = Types.ConvertTypeForMem(FD->getType(), false, true);
 if (!FD->isBitField()) return Type;

Can you add comments for the `false` and `true` parameters, e.g. 
`/*ForBitField*/ false, /*EnforceFixedLengthSVEAttribute*/ true`



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3731
   if (!Ty)
-Ty = getTypes().ConvertTypeForMem(ASTTy);
+Ty = getTypes().ConvertTypeForMem(ASTTy, false, true);
 

same here.



Comment at: clang/lib/CodeGen/CodeGenTypes.cpp:81
+llvm::Optional
+CodeGenTypes::getFixedSVETypeForMemory(const Type *T) {
+  unsigned VectorSize;

nit: `s/getFixedSVETypeForMemory/getFixedLengthSVETypeForMemory/`



Comment at: clang/lib/CodeGen/CodeGenTypes.cpp:94
+  case BuiltinType::SveUint8:
+  case BuiltinType::SveBool:
+MemEltTy = llvm::Type::getInt8Ty(Context);

Can you add a comment explaining why `SveBool` gets an `i8` element type for 
it's memory type?



Comment at: clang/lib/CodeGen/CodeGenTypes.h:137
   /// memory representation is usually i8 or i32, depending on the target.
-  llvm::Type *ConvertTypeForMem(QualType T, bool ForBitField = false);
+  llvm::Type *ConvertTypeForMem(QualType T, bool ForBitField = false,
+bool EnforceFixedLengthSVEAttribute = false);

Can you add a comment here to explain what EnforceFixedLengthSVEAttribute does?


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

https://reviews.llvm.org/D83553



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

[PATCH] D83553: [PATCH 3/4][Sema][AArch64] Add codegen for arm_sve_vector_bits attribute

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

What's the tradeoff of representing these in IR as vscale'ed vector types, as 
opposed to fixed-wdith vector types?


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

https://reviews.llvm.org/D83553



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

[PATCH] D83553: [PATCH 3/4][Sema][AArch64] Add codegen for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes updated this revision to Diff 277043.
c-rhodes added a comment.

Changes:

- Use fixed-length instead of fixed-width in naming.


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

https://reviews.llvm.org/D83553

Files:
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenTypes.cpp
  clang/lib/CodeGen/CodeGenTypes.h
  clang/test/Sema/attr-arm-sve-vector-bits-bitcast.c
  clang/test/Sema/attr-arm-sve-vector-bits-call.c
  clang/test/Sema/attr-arm-sve-vector-bits-cast.c
  clang/test/Sema/attr-arm-sve-vector-bits-codegen.c
  clang/test/Sema/attr-arm-sve-vector-bits-globals.c
  clang/test/Sema/attr-arm-sve-vector-bits-types.c

Index: clang/test/Sema/attr-arm-sve-vector-bits-types.c
===
--- /dev/null
+++ clang/test/Sema/attr-arm-sve-vector-bits-types.c
@@ -0,0 +1,525 @@
+// REQUIRES: aarch64-registered-target
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -D__ARM_FEATURE_SVE_BITS=128 -fallow-half-arguments-and-returns -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-128
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -D__ARM_FEATURE_SVE_BITS=256 -fallow-half-arguments-and-returns -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-256
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -D__ARM_FEATURE_SVE_BITS=512 -fallow-half-arguments-and-returns -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-512
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -D__ARM_FEATURE_SVE_BITS=1024 -fallow-half-arguments-and-returns -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-1024
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -D__ARM_FEATURE_SVE_BITS=2048 -fallow-half-arguments-and-returns -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-2048
+
+#include 
+
+#define N __ARM_FEATURE_SVE_BITS
+
+typedef svint8_t fixed_int8_t __attribute__((arm_sve_vector_bits(N)));
+typedef svint16_t fixed_int16_t __attribute__((arm_sve_vector_bits(N)));
+typedef svint32_t fixed_int32_t __attribute__((arm_sve_vector_bits(N)));
+typedef svint64_t fixed_int64_t __attribute__((arm_sve_vector_bits(N)));
+
+typedef svuint8_t fixed_uint8_t __attribute__((arm_sve_vector_bits(N)));
+typedef svuint16_t fixed_uint16_t __attribute__((arm_sve_vector_bits(N)));
+typedef svuint32_t fixed_uint32_t __attribute__((arm_sve_vector_bits(N)));
+typedef svuint64_t fixed_uint64_t __attribute__((arm_sve_vector_bits(N)));
+
+typedef svfloat16_t fixed_float16_t __attribute__((arm_sve_vector_bits(N)));
+typedef svfloat32_t fixed_float32_t __attribute__((arm_sve_vector_bits(N)));
+typedef svfloat64_t fixed_float64_t __attribute__((arm_sve_vector_bits(N)));
+
+typedef svbfloat16_t fixed_bfloat16_t __attribute__((arm_sve_vector_bits(N)));
+
+typedef svbool_t fixed_bool_t __attribute__((arm_sve_vector_bits(N)));
+
+//===--===//
+// Structs and unions
+//===--===//
+#define DEFINE_STRUCT(ty) \
+  struct struct_##ty {\
+fixed_##ty##_t x; \
+  } struct_##ty;
+
+#define DEFINE_UNION(ty) \
+  union union_##ty { \
+fixed_##ty##_t x;\
+  } union_##ty;
+
+DEFINE_STRUCT(int8)
+DEFINE_STRUCT(int16)
+DEFINE_STRUCT(int32)
+DEFINE_STRUCT(int64)
+DEFINE_STRUCT(uint8)
+DEFINE_STRUCT(uint16)
+DEFINE_STRUCT(uint32)
+DEFINE_STRUCT(uint64)
+DEFINE_STRUCT(float16)
+DEFINE_STRUCT(float32)
+DEFINE_STRUCT(float64)
+DEFINE_STRUCT(bfloat16)
+DEFINE_STRUCT(bool)
+
+DEFINE_UNION(int8)
+DEFINE_UNION(int16)
+DEFINE_UNION(int32)
+DEFINE_UNION(int64)
+DEFINE_UNION(uint8)
+DEFINE_UNION(uint16)
+DEFINE_UNION(uint32)
+DEFINE_UNION(uint64)
+DEFINE_UNION(float16)
+DEFINE_UNION(float32)
+DEFINE_UNION(float64)
+DEFINE_UNION(bfloat16)
+DEFINE_UNION(bool)
+
+//===--===//
+// Global variables
+//===--===//
+fixed_int8_t global_i8;
+fixed_int16_t global_i16;
+fixed_int32_t global_i32;
+fixed_int64_t global_i64;
+
+fixed_uint8_t global_u8;
+fixed_uint16_t global_u16;
+fixed_uint32_t global_u32;
+fixed_uint64_t global_u64;
+
+fixed_float16_t global_f16;
+fixed_float32_t global_f32;
+fixed_float64_t global_f64;
+
+fixed_bfloat16_t global_bf16;
+
+fixed_bool_t global_bool;
+
+//===--===//
+// Global arrays
+//===--===//
+fixed_int8_t global_arr_i8[3];
+fixed_int16_t global_arr_i16[3];
+fixed_int32_t global_arr_i32[3];
+fixed_int64_t global_arr_i64[3];
+

[PATCH] D83553: [PATCH 3/4][Sema][AArch64] Add codegen for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes created this revision.
c-rhodes added reviewers: sdesmalen, rsandifo-arm, efriedma, cameron.mcinally, 
ctetreau.
Herald added subscribers: danielkiss, kristof.beyls, tschuett.
Herald added a reviewer: rengolin.
Herald added a project: clang.

This patch implements codegen for the 'arm_sve_vector_bits' type
attribute, defined by the Arm C Language Extensions (ACLE) for SVE [1].
The purpose of this attribute is to define fixed-length (VLST) versions
of existing sizeless types (VLAT).

Implemented in this patch is the lowering of VLSTs to valid types.
VLSTs (unlike VLATs) can be used in globals, members of structs
and unions, and arrays. To support this in this patch we lower VLSTs to
arrays. For example, in the following C code:

  #if __ARM_FEATURE_SVE_BITS==512
  typedef svint32_t fixed_svint32_t __attribute__((arm_sve_vector_bits(512)));
  struct struct_int32 {
fixed_int32_t x;
  } struct_int32;
  #endif

the struct is lowered to:

  %struct.struct_int32 = type { [16 x i32] }

where the member 'x' is a fixed-length variant of 'svint32_t' that
contains exactly 512 bits.

When loading from a VLST to a VLAT, or when storing a VLAT to a VLST,
the address is bitcasted, e.g.

  bitcast [N x i8]* %addr.ptr to *

[1] https://developer.arm.com/documentation/100987/latest


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D83553

Files:
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenTypes.cpp
  clang/lib/CodeGen/CodeGenTypes.h
  clang/test/Sema/attr-arm-sve-vector-bits-bitcast.c
  clang/test/Sema/attr-arm-sve-vector-bits-call.c
  clang/test/Sema/attr-arm-sve-vector-bits-cast.c
  clang/test/Sema/attr-arm-sve-vector-bits-codegen.c
  clang/test/Sema/attr-arm-sve-vector-bits-globals.c
  clang/test/Sema/attr-arm-sve-vector-bits-types.c

Index: clang/test/Sema/attr-arm-sve-vector-bits-types.c
===
--- /dev/null
+++ clang/test/Sema/attr-arm-sve-vector-bits-types.c
@@ -0,0 +1,525 @@
+// REQUIRES: aarch64-registered-target
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -D__ARM_FEATURE_SVE_BITS=128 -fallow-half-arguments-and-returns -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-128
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -D__ARM_FEATURE_SVE_BITS=256 -fallow-half-arguments-and-returns -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-256
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -D__ARM_FEATURE_SVE_BITS=512 -fallow-half-arguments-and-returns -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-512
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -D__ARM_FEATURE_SVE_BITS=1024 -fallow-half-arguments-and-returns -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-1024
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -D__ARM_FEATURE_SVE_BITS=2048 -fallow-half-arguments-and-returns -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-2048
+
+#include 
+
+#define N __ARM_FEATURE_SVE_BITS
+
+typedef svint8_t fixed_int8_t __attribute__((arm_sve_vector_bits(N)));
+typedef svint16_t fixed_int16_t __attribute__((arm_sve_vector_bits(N)));
+typedef svint32_t fixed_int32_t __attribute__((arm_sve_vector_bits(N)));
+typedef svint64_t fixed_int64_t __attribute__((arm_sve_vector_bits(N)));
+
+typedef svuint8_t fixed_uint8_t __attribute__((arm_sve_vector_bits(N)));
+typedef svuint16_t fixed_uint16_t __attribute__((arm_sve_vector_bits(N)));
+typedef svuint32_t fixed_uint32_t __attribute__((arm_sve_vector_bits(N)));
+typedef svuint64_t fixed_uint64_t __attribute__((arm_sve_vector_bits(N)));
+
+typedef svfloat16_t fixed_float16_t __attribute__((arm_sve_vector_bits(N)));
+typedef svfloat32_t fixed_float32_t __attribute__((arm_sve_vector_bits(N)));
+typedef svfloat64_t fixed_float64_t __attribute__((arm_sve_vector_bits(N)));
+
+typedef svbfloat16_t fixed_bfloat16_t __attribute__((arm_sve_vector_bits(N)));
+
+typedef svbool_t fixed_bool_t __attribute__((arm_sve_vector_bits(N)));
+
+//===--===//
+// Structs and unions
+//===--===//
+#define DEFINE_STRUCT(ty) \
+  struct struct_##ty {\
+fixed_##ty##_t x; \
+  } struct_##ty;
+
+#define DEFINE_UNION(ty) \
+  union union_##ty { \
+fixed_##ty##_t x;\
+  } union_##ty;
+
+DEFINE_STRUCT(int8)
+DEFINE_STRUCT(int16)
+DEFINE_STRUCT(int32)
+DEFINE_STRUCT(int64)
+DEFINE_STRUCT(uint8)
+DEFINE_STRUCT(uint16)
+DEFINE_STRUCT(uint32)
+DEFINE_STRUCT(uint64)
+DEFINE_STRUCT(float16)
+DEFINE_STRUCT(float32)
+DEFINE_STRUCT(float64)
+DEFINE_STRUCT(bfloat16)
+DEFINE_STRUCT(bool)
+
+DEFINE_UNION(int8)