[cfe-commits] [PATCH] ARM AAPCS-VFP: fix handling of homogeneous aggregate.

[manman ren]

On Oct 29, 2012, at 2:38 PM, Eli Friedman <eli.friedman at gmail.com> wrote:

> On Mon, Oct 29, 2012 at 10:16 AM, manman ren <mren at apple.com> wrote:
>> 
>> If Homogeneous Aggregate can only partially fit into VFP registers, we add padding to make sure
>> HA will be on stack and later VFP CPRCs will be on stack as well.
>> 
>> There is a discussion on whether we should use byval to pass things on stack:
>> llvm-commits: ABI: how to let the backend know that an aggregate should be allocated on stack
>> 
>> For now, the patch implemented what Eli suggested.
> 
> +      llvm::Type *PaddingTy = llvm::VectorType::get(
> +          llvm::Type::getFloatTy(getVMContext()), NumVFPs - PreAllocation);
> 
> Using illegal vector types here isn't a good idea, even if it may work
> for the moment.

Hi Eli,

Thanks for reviewing.
Do you have any suggestion on what type to use as padding here?

> 
> +      it->info = ABIArgInfo::getDirect(0, 0, PaddingTy);
> 
> getExpandWithPadding would be a bit more clear here.
Will do.
> 
> +        AllocatedVFP = llvm::RoundUpToAlignment(AllocatedVFP,
> +                       getContext().getTypeSize(Base) == 64 ? 2 : 4);
> +        AllocatedVFP += Members * (getContext().getTypeSize(Base) == 64 ? 2:4);
> 
> Use a separate variable for "getContext().getTypeSize(Base) == 64 ?
> 2:4".  Also, I'm not sure we can actually assume all vectors are
> either 32-bit or 64-bit.
Will use a separate variable. 
"Base" here is from isHomogeneousAggregate:
    // Homogeneous aggregates for AAPCS-VFP must have base types of float,
    // double, or 64-bit or 128-bit vectors.

> 
> +      else {
> +        AllocatedVFP = llvm::RoundUpToAlignment(AllocatedVFP, 2);
> +        AllocatedVFP += Members * 2; // Base type is double.
> +      }
> 
> Please assert that the base type is double or long double.
Will do.

Thanks,
Manman
> 
> -Eli