[cfe-commits] [PATCH] struct{bool} -> int function argument coercion emits an invalid store on ARM

[Eli Friedman]

On Thu, Feb 9, 2012 at 12:19 AM, Evgeniy Stepanov
<eugeni.stepanov at gmail.com> wrote:
> Thanks for taking a look!
> Comments inline, new patch attached.
>
> On Thu, Feb 9, 2012 at 9:34 AM, Eli Friedman <eli.friedman at gmail.com> wrote:
>> Indentation generally looks strange... maybe some stray tabs?
>
> Fixed.
>
>> +         llvm::Value *Casted =
>> +           Builder.CreateBitCast(TempAlloca,
>> llvm::PointerType::getUnqual(ConvertTypeForMem(Ty)));
>> +         llvm::LoadInst *Load = Builder.CreateLoad(Casted);
>> +         Builder.CreateStore(Load, Ptr);
>>
>> Please use memcpy to copy structs, not load+store.
>
> Done.
>
>> -      if (llvm::StructType *STy =
>> -            dyn_cast<llvm::StructType>(ArgI.getCoerceToType())) {
>> -        Ptr = Builder.CreateBitCast(Ptr, llvm::PointerType::getUnqual(STy));
>> +      llvm::StructType *STy =
>> dyn_cast<llvm::StructType>(ArgI.getCoerceToType());
>> +      if (STy && STy->getNumElements() > 1) {
>>
>> For the case where STy->getNumElements() == 1, do we avoid storing a
>> struct type?  If not, can you fix that?
>
> Yes, as far as I can see. We go into CreateCoercedStore, then
> EnterStructPointerForCoercedAccess.

Okay.

+
+          Builder.CreateMemCpy(Ptr, TempAlloca, DstSize, AlignmentToUse);

It looks like you don't actually ensure that TempAlloca is sufficiently aligned.

Otherwise, looks fine.

-Eli