[cfe-commits] [PATCH] AArch64 backend: Clang changes

[Tim Northover]

Hi John,

Thanks again for your reviews. This is mostly a "will fix" message,
with a few extra comments after some investigation.

>      assert((E->getObjectKind() == OK_ObjCProperty ||
> +            CT == Ctx.getCanonicalType(Ctx.getBuiltinVaListType()) ||
>              !Ctx.getLangOpts().CPlusPlus) &&
>             "C++ struct assignment should be resolved by the "
>             "copy assignment operator.");

> This is a fairly suspicious change.  Why are you ending up in this code for
> struct __va_list in C++?

It appears to be no longer necessary. It used to be the path followed
for certain va_arg invocations in C++ mode (hit when building Clang
with itself). Removed now.

> Why not just make a function to mangle the vector element (returning
> "Poly8" or "Uint16" or whatever) and then construct the rest of the mangling
> programmatically?

Good idea. It'll be in the next patch.

> You've written the assert condition twice.  Just assert.
> Prefer to write this the other way around [Re: Triple ==]
> Spurious whitespace change.
> Just don't implement the function.
> This should getAs<BuiltinType>() and check the kind [...]
> If vectors of 'double' are only acceptable in AArch64, you need to be testing for that

Thanks. Corrected.

> It seems to be the case that aarch64's ABI is just the ARM32 ABI except
> that ABI guards are 64-bit.  A cleaner way of expressing this would be to
> say that aarch64 uses the ARM ABI, and that the ARM ABI says that ABI
> guards are size_t's.  Are you expecting further divergences?

It would be a simpler change and we're certainly not anticipating any
more differences on the AArch64 side. This was actually an attempt to
improve existing code; I didn't like "IsARM" covering multiple
different company-specific traits that other targets might want to
change.

However, I'm quite happy to keep to the simple one. My next patch will
make the change you suggest.

> Can I kibbitz about your ABI here, or is it too late and you've actually
> committed to it?

I'm afraid it is too late for changes now, barring complete breakage
being discovered. The specification is published and implemented by at
least two separate compilers (GCC and ARMCC).

> I will note that your va_arg code is *huge*.

It's a similar size to the x86_64 code. I suspect the complexity is
inevitable to manage the possibility of arguments going in either
general registers, floating registers or on the stack independently.

AAPCS gets around it by requiring soft-float for variadic functions so
that a single pointer suffices in va_list.

Cheers.

Tim.