[llvm-commits] [llvm] r106952 - in /llvm/trunk: include/llvm/CodeGen/ISDOpcodes.h include/llvm/CodeGen/SelectionDAG.h lib/CodeGen/SelectionDAG/LegalizeDAG.cpp lib/CodeGen/SelectionDAG/LegalizeTypesGeneric.cpp lib/CodeGen/SelectionDAG/SelectionDAG

[Rafael Espindola]

On 27 June 2010 11:30, Duncan Sands <baldrick at free.fr> wrote:
> Hi Rafael
>
>>>> +  const unsigned Align = std::max(OldAlign, TypeAlign);
>>>
>>> I don't see the point of taking the maximum here: the alignment of the
>>> memory
>>> address did not change...
>>
>> This is an issue that Dan noticed: What happens for example with a
>> i128, we would split the va_arg twice and lose the alignment
>> information of the original one.
>
> I don't see the connection between your remark, and mine about why you
> take the maximum.

If I don't take the maximum, what happens in the i128 case is
(assuming i128 is 128 bits aligned)

*) In the first split, we get one VAARG aligned to 128 bits and the
second one without alignment.
*) In the second splits, we get four VAARGs, the first one aligned to 64 bits

Taking the maximum produces the expected result: four VAARG, the first
one aligned to 128 bits.

>> This works on the assumption that what we are splitting is contiguous
>> in memory, so the alignment will be equal to the ABI alignment of that
>> type, no?
>
> Only if the original alignment was not less than the ABI alignment.  How do
> you know that?

Because that is not supported. If it was, setting the alignment using
max as we do above would not work. Also, since the elements are
assumed to be continuous in memory, we don't want any padding to be
added, which is exactly what the alignment info is used for.

It you want to avoid the max, it should be possible to

*) When creating the initial VAARG nodes, set the alignment (instead
of keeping it at zero) if extra alignment code is know to be needed
(true for i64, false for i32).
*) When splitting, keep the alignment in the first element. The ones
in the remaining VAARGs go to zero.

In this way having alignment set to 0 would just be saying "this is as
aligned as it should be, don't add extra code".

Note that for example in the test I added we only align once with an
"add r0, r0, 7; and r0, r0, -8". If both VAARG nodes had alignment
information we would add an extra "add r0, r0, 3; and r0, r0, -4".

> Ciao,
>
> Duncan.
>


Cheers,
-- 
Rafael Ávila de Espíndola