intel / intel/llvm

The class `ext::oneapi::bfloat16` has too greedy and thus wrong definition of math/relative operators

Open
#11,244 17 comments 0 reactions 0 assignees View on GitHub
bug confirmed
Dominant language
LLVM
Stars
1.5k
Forks
854
Avg merge
3d 17h
Merged PRs (30d)
137

Description

The issue was discovered when I tried to compare ext::intel::esimd::simd< bfloat16, N > with bfloat16.

Such comparison must return ext::intel::esimd::simd_mask< N > and the corresponding operator<() is defined inside esimd namespace.
The attempt to compile such code resulted into 'ambiguous call' error.

One candidate is the operator<() returning simd_mask< N > defined in `esimd` namespace and another candidate is defined in ext/oneapi/bfloat16.hpp

I can generalize the problem to any user type `T` as below:
1) bfloat16.hpp (shown below) defines math and comparison operators for user type `T` without user consent, which results into non-avoidable 'ambiguous call' errors if user want to re-define operators, e.g. `operator(const T&, const bfloat16&)`.
2) The declaration shown below does not follow C++ type-promotion-rules choosing heavier type. E.g. `(double + bfloat16)` must result into `double`, **not** `bfloat16` as defined below.

```c++
// Binary operators overloading
#define OP(type, op) \
friend type operator op(const bfloat16 &lhs, const bfloat16 &rhs) { \
return type{static_cast(lhs) op static_cast(rhs)}; \
} \
template \
friend type operator op(const bfloat16 &lhs, const T &rhs) { \
return type{static_cast(lhs) op static_cast(rhs)}; \
} \
template \
friend type operator op(const T &lhs, const bfloat16 &rhs) { \
return type{static_cast(lhs) op static_cast(rhs)}; \
}
OP(bfloat16, +)
OP(bfloat16, -)
OP(bfloat16, *)
OP(bfloat16, /)
OP(bool, ==)
OP(bool, !=)
OP(bool, <)
OP(bool, >)
OP(bool, <=)
OP(bool, >=)
#undef OP
```

In particular, this function:
```c++
template \
friend type operator op(const T &lhs, const bfloat16 &rhs) { \
return type{static_cast(lhs) op static_cast(rhs)}; \
}
```
blindly tries to convert any object 'const T &lhs' to 'float', then compare it with 'rhs', then return 'bool'.
This behavior conflicts with any alternative definitions such as 'esimd::simd' and any potential user's class T.

Users can easily want to have some class defining 'operator<()' differently, for example:
```c++
struct Ray {
float x;
float y;
};
bool operator<(const Ray &R, const bfloat16 &Length) { return sqrt(R.x*R.x + R.y*R.y) < Length; }
Ray operator+(const Ray &R, const bfloat16 &Extra) {/*some tricky code updating X and Y of the new vector making it a bit longer than 'R'*/}
```
Such code would get same 'ambiguous call' error
or an error during the attempt to 'static_cast< float >' of user's object that is done at bfloat16.hpp header.

The better and more safe approach is used for 'sycl::half' type defined inside 'sycl/half_type.hpp':
https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/half_type.hpp#L449-L524

- OS: Windows and Linux
- Target device and vendor: Intel GPU, all other devices that support ext::oneapi::bfloat16 type

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.