[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 = &global;
>   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 <vscale x 4 x i32>*, 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, <vscale x 4 x i32>** %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 = &arr[0];
>   return array_ptr;
> }```
> 
> trying to create a store:
> `store [4 x i32]* %0, <vscale x 4 x i32>** %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