<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jan 19, 2016 at 3:16 PM, Eddy B. <span dir="ltr"><<a href="mailto:edy.burt@gmail.com" target="_blank">edy.burt@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">> Do all tests need it? Align is just for non-default alignment (> align(1)) so it may be OK for the tests to omit it & it's probably not right to /require/ an align attribute on a byval parameter.<br>
<br>
</span>I've had to fix some of the tests which checked that the right<br>
alignment was applied,<br></blockquote><div><br></div><div>Rough order of magnitude number of tests that needed to be fixed? (10s? 100s?)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
by looking at the generated asm, but had no "align A" attributes and<br>
relied on the<br>
target alignment guessing, which I had taken out in my experimental<br>
implementation.<br>
<br>
The rest of the tests might be fine, but could behave surprisingly<br>
upon closer inspection.<br></blockquote><div><br></div><div>I'd generally be OK with this ^</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
2016-01-20 1:09 GMT+02:00 David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>>:<br>
> +Chandler, Duncan, David M, and Reid (pseudorandom selection of people who<br>
> might be interested/voiced opinions on prior threads/conversations on the<br>
> subject)<br>
><br>
> Firstly, thanks Eddy for doing a lot of work in the opaque pointer area -<br>
> it's greatly appreciated. This is a particularly thorny part of it (or, at<br>
> least I think so at the moment - sounds like you've seen thornier parts<br>
> further down the trail).<br>
><br>
> On Tue, Jan 19, 2016 at 2:47 PM, Eddy B. via llvm-dev<br>
> <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br>
>><br>
>> Hi,<br>
>><br>
>> In the past months, several options have been presented for making byval<br>
>> (and similar attributes, such as inalloca or sret) work with opaque<br>
>> pointers.<br>
>><br>
>> The main two I've seen were byval(T) and byval(N) where N is the size of<br>
>> T.<br>
>><br>
>> They both have their upsides and downsides, for example: byval(T) would be<br>
>> a type-parametric attribute, which, AFAIK, does not already exist and may<br>
>> complicate the attribute system significantly, while byval(N) would be<br>
>> hard<br>
>> to introduce in tests as computing N from T requires LLVM's DataLayout.<br>
><br>
><br>
> By "hard to introduce in tests" you just mean it's hard to edit the test<br>
> cases to add the correct value of N automatically? Yeah - it's certainly not<br>
> a simple sed script like my previous transformations.<br>
><br>
>><br>
>> Also, this would have to be done for inalloca and sret as well - sret only<br>
>> needs it when targeting SPARC, although still generally useful in<br>
>> analysis.<br>
><br>
><br>
> Reid, Dave - bringing this to your attention in case you've got any thoughts<br>
> on inalloca/sret (was sret part of the Windows-y things, or just inalloca?),<br>
> how they should be handled, if it should be different/the same as byval,<br>
> etc?<br>
><br>
>><br>
>> To sidestep some of the concerns and allow a smooth transition towards a<br>
>> byval that works with opaque pointers, I've come up with a new approach:<br>
>><br>
>> Reuse dereferenceable(S) and align A for the size and alignment of byval.<br>
><br>
><br>
> Side note: align should already be what's used for the alignment of byval. I<br>
> think I remember seeing that there was one target hook and one target using<br>
> that hook, that did some backend alignment fixes. If that is still present,<br>
> we should fix/remove it so that the align attribute is the one source of<br>
> truth for the alignment of a byval parameter. This should happen before any<br>
> changes to byval.<br>
><br>
> The dereferenceable part is the novelty here and I really don't know how<br>
> everyone feels about it. I don't much mind - I suppose I like it because it<br>
> means optimizations can, maybe, hack on byval parameters more effectively<br>
> without having to have byval-specific smarts necessarily. (also, because<br>
> you've already done a bunch of the work)<br>
><br>
>><br>
>> That is, a byval dereferenceable(S) align A argument is guaranteed to have<br>
>> S bytes available to read from, *and only S*, aligned to a multiple of A.<br>
>> Reading past that size is UB, as LLVM will not copy more than S bytes.<br>
>><br>
>> An API can be provided to add the attribute alongside dereferenceable<br>
>> and align attributes, for a given Type* and DataLayout.<br>
>><br>
>> A preliminary implementation (w/o sret) can be found at:<br>
>> <a href="https://github.com/eddyb/llvm/compare/2579466...65ac99b" rel="noreferrer" target="_blank">https://github.com/eddyb/llvm/compare/2579466...65ac99b</a><br>
>><br>
>> To maintain compatibility with existing code,<br>
><br>
><br>
> ^ this isn't actually a goal/necessity, fwiw. So we might not need the<br>
> complexity you're outlining below. Or if we do choose to take it, we may<br>
> only want/need to keep it around for a relatively short period.<br>
><br>
>><br>
>> dereferenceable and align<br>
>> attributes are automatically injected as soon as a non-default DataLayout<br>
>> is available. The "injection" mechanism could potentially be replaced with<br>
>> a pass, although it was easier to experiment with it being guaranteed.<br>
>><br>
>> This works out pretty well in practice, as analysis already understands<br>
>> dereferenceable and can make decisions based on it.<br>
>><br>
>> The verifier checks that for byval & friends, dereferenceable(S) and<br>
>> align A are present (clang always adds align, but not all tests have it)<br>
><br>
><br>
> Do all tests need it? Align is just for non-default alignment (> align(1))<br>
> so it may be OK for the tests to omit it & it's probably not right to<br>
> /require/ an align attribute on a byval parameter.<br>
><br>
>><br>
>> and that S is the exact size of the pointee type (while we still know<br>
>> that).<br>
>><br>
>> That last bit is very important, because it allows a script to do the<br>
>> following:<br>
>><br>
>> 1. Find all byval arguments in tests that are missing dereferenceable,<br>
>> e.g.<br>
>>     ... i32* byval align 4 ...<br>
>>     .... {i8, i64}* byval ...<br>
>> 2. Add a bogus dereferenceable(unique ID) to each of them, i.e.<br>
>>     ... i32* byval dereferenceable(123400001) align 4 ...<br>
>>     .... {i8, i16}* byval dereferenceable(123400002) ...<br>
>> 3. Run the tests and record the errors, which may look like:<br>
>><br>
>> Attribute 'byval' expects 'dereferenceable(4)' for type i32*,<br>
>>     found 'dereferenceable(123400001)'<br>
>><br>
>> Attribute 'byval' expects 'dereferenceable(16) align 8' for type {i8,<br>
>> i64}*,<br>
>>     found 'dereferenceable(123400002)'<br>
>><br>
>> 4. Use the verifier error messages to replace the bogus attributes<br>
>> with the proper ones, which include align A when it is missing:<br>
>>     ... i32* byval dereferenceable(4) align 4 ...<br>
>>     .... {i8, i16}* byval dereferenceable(16) align 8 ...<br>
>><br>
>> For what is worth, the same scheme would also work for byval(N), and<br>
>> would be entirely unnecessary for byval(T).<br>
>><br>
>> I would love to know your thoughts on this, and more specifically:<br>
>> Which of the 3 (byval(T), byval(N) and byval + dereferenceable + align)<br>
>> do you think would provide the easiest transition path for front-ends?<br>
>><br>
>> Thank you,<br>
>>  - eddyb<br>
>> _______________________________________________<br>
>> LLVM Developers mailing list<br>
>> <a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a><br>
>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>