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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
