[cfe-commits] [PATCH] Add PNaCl ABIInfo

[David Meyer]

John,

> Correct me if I'm wrong, but this "ABI" is a recent invention of yours that
> does not currently affect the binary compatibility of any released software.
> It is apparently specified directly in terms of what is otherwise an
> internal implementation detail of Clang — namely, the IR type that we
> translate a struct into.  As we've brought up many times already, this
> translation is very lossy and will not be adequate for your purposes
> without significant embellishments of the LLVM type system which
> many core developers would not be comfortable with.

How Clang translates a struct to IR is not an internal implementation
detail of Clang. It is very much driven by the target ABI. In many
cases, you have no choice. The platform ABI forces you to lower
structs in a certain way.

This is one of the reasons why I wish bitcode were already platform
independent. It should *not* be the responsibility of bitcode
consumers (like us) to convince bitcode producers that our
representation is worthy of emission.

> In short, several of us are objecting to your choice of ABI, which we do
> have the right to do, since that ABI is intrinsically intended to be a
> permanent constraint on LLVM and Clang.

I have not seen an objection from anybody else besides you, and Eli
has already approved this change. What is the procedure for resolving
this kind of dispute?

>
> So, first off, your current method doesn't satisfy all of your requirements,
> because it's not capable of being correctly translated into
> target-dependent IR.  So there is really no reason to privilege it.
>

As I mentioned in the other thread, we don't require full ABI
compatibility, only partial ABI compatibility. We're reasonably
confident we can meet these requirements with minor tweaks to the
current representation.

> But second off, I have already proposed passing aggregates byval,
> which is completely equivalent to your current approach except for not
> needing first-class aggregates.  Your target-specific translation pass can
> rewrite byval arguments just as well as it can rewrite FCA arguments.
> Of course, this suffers from the exact same problem of relying on the
> inadequately expressive LLVM type system.

I believe it would be less efficient to rewrite byval in the backend.
The semantics of byval imply that a pointer to the struct will be
available. This would require us to always allocate a stack slot for
the argument, which destroys the performance benefits of register
passing.

It's true we have to do this stack allocation anyway in the IR
generated by the frontend, but at least when it is done there, it will
be optimized out if it is not needed.

Even though we could switch to byval, we really need a better argument
for making such a shift, beyond just minimizing the patch to Clang.

>
>> While our ABI is not 100% finalized, until this patch is committed, we
>> can't do any external testing with Clang. (unless we create our own
>> branch, which we'd rather not)
>
> I'm sorry, but we do expect code to go through code review, which
> includes a thorough review of the underlying motivations and technical
> design.

This doesn't make sense. Ultimately, the PNaCl project controls its
own ABI. And I don't mean me. I was not involved in the decision to
use first-class struct arguments, and if that decision needs to be
altered, I'm not the person who has the final say on it.

Anybody on the Clang team is welcome to offer advice for PNaCl, but
you must first appreciate the strange technical requirements we have
to satisfy, and that requires learning about the NaCl environment and
how we fit into the rest of the NaCl project.

And I'm sorry we can't guarantee this code won't change. I had no idea
the Clang codebase was so.. immaculate? Our project is a work in
progress. That means we sometimes check in code which is not
feature-complete, and possibly even contains bugs.

- pdox