[llvm-commits] patch: fix emission of npot vectors which need trailing padding

[Nick Lewycky]

Hi Renato! Sorry, but I don't understand your reply at all.

The correctness of this patch is dependent on whether an LLVM vector should
ever have trailing padding (not element padding). If the answer is no
regardless of platform, then we should add an assertion in this spot instead
of code to write out padding.

There's nothing ARM-specific about this change, which is why it belongs in
generic CodeGen and shouldn't be in the ARMAsmPrinter. It just so happens
that <3 x float> on ARM provides a counter-example that would require this
padding or trigger this assertion.

If we agree that they should never be padded, then the backend would be
responsible for either expanding the <3 x float> into a <4 x float>, or else
breaking it into <2 x float> + float. Is that what you're arguing for?

Alternatively, if you want <3 x float> to be 12 bytes and densely packed,
that'd be an independent change. You'd want to modify what TargetData says
about the <3 x float>, so that the computation performed by GEP, etc., all
line up. Currently I'm just fixing EmitGlobalConstantVector to emit n-bytes
when target data says the vector is n-bytes long.

Nick

On 21 June 2011 01:45, Renato Golin <renato.golin at arm.com> wrote:

> On 21 June 2011 06:52, Nick Lewycky <nlewycky at google.com> wrote:
> > This lowers to a struct with <3 x float>, which TargetData says is 16
> bytes
> > on ARM.
>
> Hi Nick,
>
> This only makes sense for ARM with NEON/VFP, unless selection-DAG
> identifies it and remove the padding to avoid extra instructions being
> generated for the CPU when NEON/VFP is not present.
>
> Anyone knows if that's the case?
>
>
> > In LLVM IR, the first element becomes a constant aggregate zero,
> > which is emitted by looking at how many bytes it is (sixteen) and writing
> > out a zero-fill. The second element is emitted as three contiguous
> elements,
> > resulting in a total of 12 bytes.
>
> I agree, we should have consistency here.
>
>
> > The StructLayout object claims that each of those should be 16 bytes, so
> I
> > changed the AsmPrinter in CodeGen to apply padding to the end of an array
> to
> > match what TargetData thinks the length of the array is. Patch attached,
> > please review!
>
> Not sure you changed the right place. I agree that alignment is
> important for all platforms but I wouldn't make such an assumption
> without a set of tests for each one of them.
>
> If everyone is happy that such padding needs to be dealt with by the
> generic CodeGen, I won't oppose, but would be good to have at least
> one test for each major architecture.
>
> Otherwise, if you move it to ARMAsmPrinter, I still suggest you to
> write a simple IR-to-ASM test and check data sizes.
>
> cheers,
> --renato
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110621/cd2ddf05/attachment.html>