[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 = &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.


================
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