Hi Renato! Sorry, but I don't understand your reply at all.<div><br></div><div>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.</div>


<div><br></div><div>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.</div>


<div><br></div><div>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?</div>


<div><br></div><div>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.</div>


<div><br></div><div>Nick</div><div><br><div class="gmail_quote">On 21 June 2011 01:45, Renato Golin <span dir="ltr"><<a href="mailto:renato.golin@arm.com" target="_blank">renato.golin@arm.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>On 21 June 2011 06:52, Nick Lewycky <<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>> wrote:<br>
> This lowers to a struct with <3 x float>, which TargetData says is 16 bytes<br>
> on ARM.<br>
<br>
</div>Hi Nick,<br>
<br>
This only makes sense for ARM with NEON/VFP, unless selection-DAG<br>
identifies it and remove the padding to avoid extra instructions being<br>
generated for the CPU when NEON/VFP is not present.<br>
<br>
Anyone knows if that's the case?<br>
<div><br>
<br>
> In LLVM IR, the first element becomes a constant aggregate zero,<br>
> which is emitted by looking at how many bytes it is (sixteen) and writing<br>
> out a zero-fill. The second element is emitted as three contiguous elements,<br>
> resulting in a total of 12 bytes.<br>
<br>
</div>I agree, we should have consistency here.<br>
<div><br>
<br>
> The StructLayout object claims that each of those should be 16 bytes, so I<br>
> changed the AsmPrinter in CodeGen to apply padding to the end of an array to<br>
> match what TargetData thinks the length of the array is. Patch attached,<br>
> please review!<br>
<br>
</div>Not sure you changed the right place. I agree that alignment is<br>
important for all platforms but I wouldn't make such an assumption<br>
without a set of tests for each one of them.<br>
<br>
If everyone is happy that such padding needs to be dealt with by the<br>
generic CodeGen, I won't oppose, but would be good to have at least<br>
one test for each major architecture.<br>
<br>
Otherwise, if you move it to ARMAsmPrinter, I still suggest you to<br>
write a simple IR-to-ASM test and check data sizes.<br>
<br>
cheers,<br>
<font color="#888888">--renato<br>
</font></blockquote></div><br></div>