Page MenuHomePhabricator

[Sema] Allow non-member operators for sizeless SVE types
Needs ReviewPublic

Authored by rsandifo-arm on Mar 30 2020, 7:05 AM.

Details

Summary

SVE types are defined to be opaque built-in types that by default
can only be used via intrinsics. One consequence of this is that
the types provide no built-in versions of the unary and binary
arithmetic operators.

Instead, the ACLE allows users to define non-member operators
for SVE types, in much the same way as standard C++ does for
enumerator types, and as Clang as an extension does for matrix types.
See section 3.6 of the ACLE at:
https://developer.arm.com/documentation/100987/0000/
for details.

In response to earlier feedback from the Clang community (thanks!),
the ACLE requires overloads to be static or to be defined within a
namespace if all overloadable parameter types are sizeless types.
This reduces the risk of an accidental ODR violation if two objects
use different overloads of the same operation. (That kind of ODR
violation seems more likely for sizeless types than for classes
or enums, since classes and enums are defined explicitly by a given
piece of translated code, whereas SVE types are built directly into
the compiler.)

The patch triggers a duplicate warning for:

                                                                            
ref_int8 = ref_int8;

but it seemed better to fix that separately.

Diff Detail

Event Timeline

rsandifo-arm created this revision.Mar 30 2020, 7:05 AM
Herald added a project: Restricted Project. · View Herald Transcript
rsandifo-arm edited the summary of this revision. (Show Details)Mar 30 2020, 7:08 AM

I'm concerned we're going to run into trouble if two people define different SVE "libraries", and you try to link both of them into the same program. If you're not careful, you end up with ODR violations. The scope of this is sort of restricted in normal C++: class and enumerated types sort of have an informal "owner", and people tend to respect that. I mean, you could say it's the user's own fault if they break ODR, but we're essentially making a trap here. Maybe it would make sense to require that SVE operators are defined in a namespace? That would make the user think about the issue.

We're also basically committing to never building these operators into the compiler, for all sizeless types for all targets.

It would probably make sense to send this to cfe-dev, to get more perspectives on the language change.

clang/include/clang/AST/Type.h
7049–7053

Unnecessary parentheses

clang/test/SemaCXX/sizeless-1.cpp
683

Need some testcases with template operators.

I'm concerned we're going to run into trouble if two people define different SVE "libraries", and you try to link both of them into the same program. If you're not careful, you end up with ODR violations. The scope of this is sort of restricted in normal C++: class and enumerated types sort of have an informal "owner", and people tend to respect that. I mean, you could say it's the user's own fault if they break ODR, but we're essentially making a trap here. Maybe it would make sense to require that SVE operators are defined in a namespace? That would make the user think about the issue.

Thanks for the suggestion. Requiring a namespace sounds like a good plan -- will give it some more thought.

We're also basically committing to never building these operators into the compiler, for all sizeless types for all targets.

I guess this brings up the question of when we should introduce a specific isSizelessSVEBuiltinType() query. One option would to be wait until there's at least one type that isSizelessBuiltinType but isn't an SVE type. Another would be to introduce it when a feature seems too SVE-specific. If you think this qualifies for the latter then I can add the query now.

For AArch64, it would always be possible in future to provide a standard header file that defines certain operators for SVE types. One way of implementing the header file would be to use magic attributes that get the compiler to fill in the implementation. (Or it could perhaps just use a pragma to "enable" built-in operators, but that sounds riskier.)

It would probably make sense to send this to cfe-dev, to get more perspectives on the language change.

OK. I'll address your comments first and then post there.

One option would to be wait until there's at least one type that isSizelessBuiltinType but isn't an SVE type. Another would be to introduce it when a feature seems too SVE-specific

Probably not a big deal either way, as long as we document the intent in the code.

rsandifo-arm edited the summary of this revision. (Show Details)

Require operators to be defined as static or inside a namespace

Also remove redundant brackets and add more tests.

rsandifo-arm marked 2 inline comments as done.Mar 31 2020, 6:56 AM
rsandifo-arm retitled this revision from RFC: [Sema][SVE] Allow non-member operators for SVE types to [Sema][SVE] Allow non-member operators for SVE types.May 1 2020, 10:41 AM

I'd like to promote this from an RFC to an RFA. The associated cfe-dev discussion was:

http://lists.llvm.org/pipermail/cfe-dev/2020-March/065060.html

continuing into April here:

http://lists.llvm.org/pipermail/cfe-dev/2020-April/065069.html

I don't think there were any objections to the proposal. In particular, in:

http://lists.llvm.org/pipermail/cfe-dev/2020-March/065062.html

Erich referred specifically to 'other' vector types, so it didn't sound like that was an objection to allowing overloading for these SVE types. (The SVE types aren't vectors at the language level, but are instead just opaque blobs. The patch is specific to them, and leaves the handing of normal vectors unchanged.)

Rebase onto current trunk, and also on top of:
https://reviews.llvm.org/D92222

rsandifo-arm retitled this revision from [Sema][SVE] Allow non-member operators for SVE types to [Sema] Allow non-member operators for sizeless SVE types.Nov 27 2020, 8:54 AM
rsandifo-arm edited the summary of this revision. (Show Details)

Sorry for reviving this after so long, but I'd like to bump it from an RFC to an RFA. As the new covering message says, the spec is now part of the ACLE specification.

rkruppe removed a subscriber: rkruppe.Nov 27 2020, 8:56 AM
efriedma added a subscriber: rsmith.

@rsmith, can you look at the changes to overloading? I haven't looked at that code in a very long time.

Otherwise looks fine.

The implementation looks fine to me.

That said, I'm concerned that the design of this feature will severely limit its utility. For classes and enums, operators are typically defined in an associated namespace so that they can be found by ADL. For these types, there are no associated namespaces, so user-defined operators for them can never be found by ADL, and such operators are essentially never going to work in generic code. Maybe that's not something you care too much about for the intended use cases?

Thanks for the reviews.

That said, I'm concerned that the design of this feature will severely limit its utility. For classes and enums, operators are typically defined in an associated namespace so that they can be found by ADL. For these types, there are no associated namespaces, so user-defined operators for them can never be found by ADL, and such operators are essentially never going to work in generic code. Maybe that's not something you care too much about for the intended use cases?

Yeah, we don't expect the intended use cases would depend on ADL. The main aim is to support a style of SIMD framework in which all operations are performed using operators or using non-member functions. The SIMD framework would then have two choices:

  1. If all those non-member functions belong to a namespace foo and if the SIMD framework expects users to be using namespace foo, the non-member operator overloads can simply be defined in foo and be brough into scope that way.
  2. Otherwise, the framework could handle non-member operator overloads in one of the following ways:
    • define the overloads to be static (perhaps the least likely)
    • define the overloads in an anonymous namespace
    • define the overloads in an internal namespace and make the header file use that namespace at global scope

The second choice would be awkward if a header file wanted to use the SIMD framework internally but leave the user free to use a different SIMD framework. That sort of use case would require the first choice instead. However, we expect in practice this feature will mostly be used in well-controlled environments where there is no risk of a clash between SIMD frameworks within a package. Requiring the overloads to be static or defined within a namespace is just to prevent accidental ODR violations between packages that are linked together.

Either way, I realise this isn't great style. It just seems like a practical compromise between the rock of classes having a constant size and the hard place of vectors having a variable length.

Ping. Also, please let me know if the response above doesn't answer the concerns and if you'd like more info/justification.

rsmith added a comment.Fri, Jan 8, 1:27 PM

Either way, I realise this isn't great style. It just seems like a practical compromise between the rock of classes having a constant size and the hard place of vectors having a variable length.

I don't think this is just a question of style; it's a severe limitation to language functionality. In a very broad sense, you would not be able to use these types with templates. For example, even if you define an operator< between svint8_t, you can't use std::min between two svint8_t's. And people are going to try to work around that by defining the operators first, then #includeing the algorithms in question, which will lead to ODR issues, will break when C++ modules is enabled, and so on.

It's also a little unclear to me what the motivation for this is. Do you really want to permit (for example) an operator+ between sizeless vectors that does something other than element-wise addition? If not, then why do we not instead directly provide built-in support for these operators, as we do for all our other vector types?

Either way, I realise this isn't great style. It just seems like a practical compromise between the rock of classes having a constant size and the hard place of vectors having a variable length.

I don't think this is just a question of style; it's a severe limitation to language functionality. In a very broad sense, you would not be able to use these types with templates. For example, even if you define an operator< between svint8_t, you can't use std::min between two svint8_t's. And people are going to try to work around that by defining the operators first, then #includeing the algorithms in question, which will lead to ODR issues, will break when C++ modules is enabled, and so on.

I agree that, even after disallowing definitions in the global namespace, the feature could still be abused to cause ODR violations. If that's a showstopper then I guess the patch can't go forward. But it seems like the feature would have legitimate uses too.

It's also a little unclear to me what the motivation for this is. Do you really want to permit (for example) an operator+ between sizeless vectors that does something other than element-wise addition? If not, then why do we not instead directly provide built-in support for these operators, as we do for all our other vector types?

The aim of the patch is to allow SIMD frameworks like https://github.com/google/highway to be ported to SVE.

Most C++ SIMD wrappers are class based, using member functions (including member operator functions) for most operations. std::simd is an obvious example of this. This style of framework can't be ported to length-agnostic SVE because classes must be a constant size. But frameworks like Highway instead use non-member functions and non-member operator overloads. That approach is compatible with sizeless types.

We'd also like to maintain the current separation between the SIMD library and the underlying architecture support. At it's heart, the SVE ACLE is just a set of low-level, machine-specific intrinsic functions. t's not supposed to be a new generic language extension.

I agree the separation between the compiler and the library isn't too important for things like operator+ on uniform types, since the operation only has one sensible implementation. But some areas do provide an element of choice. E.g.:

  • Should operator& be defined for float vectors?
  • Should operator% be implemented, even though it's not a native operation?
  • What operators should be defined for bfloat16_t vectors? At the moment, even scalar bfloat16_t supports very few “native” operations, but SIMD frameworks might want to provide more (now or in the future).
  • Should operator[] be defined for svbool_t, and if so, what type should it return when applied to lvalues?
  • Should mixtures of float and integer types be supported?

Leaving these decisions to the library means that the library can provide a consistent interface for multiple targets. If we pick one behaviour for SVE, the library would need to pick the same behaviour for other vector architectures or force users to live with the inconsistency.