<div dir="ltr">Hi,<div><br></div><div>+1 Oliver, in general.</div><div><br></div><div>Renato, I agree completely with your end goal. The place I disagree with you is in the process for achieving that. You seem to be advocating the "keep piling in more and more hacks until it is sufficiently horrendous that someone is forced to take the time to go rewrite it from scratch" methodology, which we both know from prior experience does not actually work. What actually happens is that the hacks pile up and people back away slowly and then more quickly from the code, until it is an unmaintained pile of rubbish. C.f. the clang driver.</div>
<div><br></div><div>We all know that the current design of IR means the frontend must encode some language-specific information in the IR in some form. Most of this information is fairly obvious to the reader and innocuous - for example using an sret* first parameter versus returning an aggregate. Some of this information is wholly horrible, such as coercing small vectors into i32's (to hint that it would be better to pass these through GPRs) and making 3-element vectors pass-by-reference instead of by value.</div>
<div><br></div><div>My point is that these decisions - on the contract between frontend and IR - are on a highly visible external interface and are often not backwards compatible. We should not be piling hacks into this area because we end up with technical debt that we have to service for years to come. There are many out of tree users that construct IR themselves from their own frontend or manually via C++ or through .ll files, and need that IR to interact with other functions via the native ABI. Having some things in a contract, such as sret* versus return-aggregate - is fine and is fairly obvious what the difference in behaviour is. Reordering arguments certainly is not - it is unintuitive and would result in a worse mess than the i32-coercion of the 3-element-vector-by-reference debacle that slyly made it into Clang and made it backwards-incompatible.</div>
<div><br></div><div>So I think Oliver's solution is the best one of the two going forward, and Anton's is an extra hack that we will have to live with for years - PCS builder or no PCS builder.</div><div><br></div>
<div>Also Anton, looking at the patch it looks to me like attributes are *already* 64-bits wide, Oliver is just using two unused bits of it. Unless I've missed something?</div><div><br></div><div>Cheers,</div><div><br>
</div><div>James</div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On 17 March 2014 10:50, Oliver Stannard <span dir="ltr"><<a href="mailto:oliver.stannard@arm.com" target="_blank">oliver.stannard@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 class=""><br>
> I'm in favour of having a PCS helper in the IRBuilder in any form or shape, but that's orthogonal to any front-end's inability to produce correct ABI code. I wouldn't want a new, generic PCS helper to be moulded based on a single ARM HA issue, but to be developed from scratch, with most PCSs in mind, including x86, Mips, PPC, AArch64, etc.<br>
<br>
</div> I'm not trying to create a more generic system for handling PCSs, and have tried to minimise the amount of code in target-independent places. If you have any suggestions to further reduce the target-independent code I have added, that would be appreciated.<br>
<div class=""><br>
> In that sense, I agree with Anton that this *very* specific problem sould be fixed in the front-end, as it has historically being done by ARM-compatible front-ends (llvm-gcc, clang, our own EDG bridge, and others), and a more generic approach should be taken to a wider audience, mixing Clang and LLVM developers in a "grand design".<br>
<br>
</div> My understanding of the reason for putting the PCS code in clang is that it is required because the LLVM backend cannot handle all types correctly. This patch expands the set of types that can be handled by the ARM backend, so some of the ARM-specific clang code is no longer necessary. Having two separate bits of code allocating arguments to registers has never struck me as a particularly robust design.<br>
<div class=""><br>
> In the end, other front-ends will have to adapt to your own implementation's specific details anyway, and it doesn't matter what kind of specific behaviour for the front-end engineer, as long as it works. The only better scenario is when there is *NO* PCS specific knowledge in a function declaration, and all of it is done by the PCS helper. Anything in between will be just another shoddy contract.<br>
<br>
</div> This patch strictly increases the set of types that the ARM backends can handle without help, so other frontends will continue to work as they currently do without changes. To generate ABI-compliant code, they still have the option to re-order arguments to get the correct back-filling, but this patch allows them to simply emit an argument with struct type, and it will be handled correctly.<br>
<br>
I agree that needing no PCS knowledge to create a function definition would be ideal, but that would require a major change given that some PCSs depend on source-language details that will not always be unambiguously represented in IR. However, this does not mean that there is no benefit to reducing the amount of PCS knowledge in the frontend, when it could instead be handled by the target-specific backend.<br>
<div class=""><br>
> I do want to see that happening, but not starting from a corner case.<br>
<br>
</div> I would also like to see that happen, but that is not what I am trying to do here.<br>
<div class="HOEnZb"><div class="h5"><br>
<a href="http://llvm-reviews.chandlerc.com/D3082" target="_blank">http://llvm-reviews.chandlerc.com/D3082</a><br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div>