This is the mail archive of the gcc-patches@gcc.gnu.org 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 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
<Michael.Collison@arm.com> 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
lightly.

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;
}
#else
#include "arm_neon.h"
namespace __detail {
 using __int32x4_t = int32x4_t;
 using __int8x16_t = int8x16_t;
}
#endif

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


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