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

[Evgeniy Stepanov]

On Fri, Feb 10, 2012 at 4:34 AM, Eli Friedman <eli.friedman at gmail.com> wrote:
> 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.

Right. Fixed and committed as r150238.

Thanks again.

>
> -Eli