This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][Aarch64] Add vectorized mersenne twister

On 06/06/17 12:33 +0100, James Greenhalgh wrote:
On Tue, Jun 06, 2017 at 11:47:45AM +0100, Jonathan Wakely wrote:
On 06/06/17 11:23 +0100, Jonathan Wakely wrote:
>On 06/06/17 11:07 +0100, James Greenhalgh wrote:
>>On Fri, Jun 02, 2017 at 07:13:17PM +0100, Jonathan Wakely wrote:
>>>On 02/06/17 19:19 +0200, Ulrich Drepper wrote:
>>>>On Fri, Jun 2, 2017 at 5:46 PM, Michael Collison
>>>><> wrote:
>>>>>This implementation includes "arm_neon.h" when including the optimized <ext/random>.  This has the effect of polluting the global namespace with the Neon intrinsics, so user macros and functions could potentially clash with them.  Is this acceptable given this only happens when <ext/random> is explicitly included?
>>>>I don't think it is.  Ideally the sfmt classes would get added to the
>>>>standard some day; they are much better performance-wise than the mt
>>>>classes.  At that point you'd have no excuse anymore.
>>>>Why are the symbols in arm_neon.h not prefixed with an underscore as
>>>>it is the case for equivalent definitions of other platforms (notably
>>>>x86)?  If you start adding such optimizations I suggest you create
>>>>headers which do not pollute the namespace and include it in
>>>>arm_neon.h and whatever other header is needed to define the symbols
>>>>needed for compatibility.
>>>>Nip the problem in the butt now, this is probably just the beginning.
>>We're a good number of years late to do that without causing some pain.
>>The arm_neon.h names for AArch64 come from those for the ARM port, which
>>themselves started out in the mid 2000s in ARM's proprietary compiler. These
>>names are now all over the place; from LLVM, through popular proprietary
>>toolchains, through a variety of developer guides and codebases, and in
>>to GCC.
>>Forging a new interface is possible, but expecting migration to the
>>namespace-safe names seems unlikely given the wide spread of the current
>>names. Essentially, we'd be doubling our maintenance, and asking all
>>other compilers to follow. It is possible, but we'd not want to do it
>>In particular, my immediate worry is needing other compilers to support the
>>new names if they are to use libstdc++.
>>If we don't mind that, and instead take a GCC-centric view, we could avoid
>>namespace polution by using the GCC-internal names for the intrinsics
>>(__builtin_aarch64_...). As those names don't form a guaranteed interface,
>>that would tie us to a GCC version.
>Libstdc++ is already tied to a GCC version when used with GCC. Using
>the internal names might cause problems when using libstdc++ with
>non-GNU compilers if they don't use the same names (it's used with at
>least Clang and ICC).
>>So we have a few solutions to choose from, each of which invokes a trade-off:
>>1 Use the current names and pollute the namespace.
>>2 Use the GCC internal __builtin_aarch64* names and tie libstdc++ to GCC
>>  internals.
>>3 Define a new set of namespace-clean names and complicate the Neon
>>  intrinsic interface while we migrate old users to new names.
>>I can see why the libstdc++ community would prefer 3) over the other options,
>>but I'm reticent to take that route as the cost to our intrinsic maintainers
>>and users looks high. I've added some of the other ARM port maintainers
>>for their opinion.
>>Are there any other options I'm missing?
>3b) Define a new subset of names for the parts needed by this patch,
>  but don't expect users to migrate to them.
>That probably doesn't have any advantage over 2.

I suppose <ext/random> could do something like:

#if __GNUC__ >= 8  // This is GCC not Clang pretending to be GCC 4.2
namespace __detail {
 using __int32x4_t = __Int32x4_t;
 using __int8x16_t = __Int8x16_t;
#include "arm_neon.h"
namespace __detail {
 using __int32x4_t = int32x4_t;
 using __int8x16_t = int8x16_t;

But we'd need to redefine or wrap the operations like veorq_u8 which
would be a pain.

Thanks for the feedback. We've got the clear message that this approach
is unacceptable, the suggestions for other possible ways forward are very

I think we got some of the way towards this when we were discussing this
internally. For many of the intrinsics needed by this patch we can get
away with using the GCC vector syntax (a ^ b for veorq_u8) and avoid
relying on the header at all. I think we ought to go back to that idea and
see how far we can take it, and which names we really do need to agree on
or mask in this way.

I presume using the GCC vector extensions (which are certainly supported
by Clang, I don't know about ICC) will be OK?

Yes, I had a look in the Clang arm_neon.h header and it uses the same
vector extensions. If all the operations needed for Michael's patch
can be done using those instead of the named functions I'd be happy
with that. The handful of typedefs needed can be defined in some
libstdc++ header, rather than by including in arm_neon.h

I assume we don't actually need to worry about ICC, because this is
ARM-only code.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]