fix compilation of uniform int distribution #2284
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2284 +/- ##
========================================
Coverage 91.45% 91.45%
========================================
Files 433 433
Lines 16175 16177 +2
========================================
+ Hits 14793 14795 +2
Misses 1382 1382
|
| // for the generator is not one of short, int, long, long long, unsigned short, | ||
| // unsigned int, unsigned long, or unsigned long long. See | ||
| // https://en.cppreference.com/w/cpp/numeric/random/uniform_int_distribution. | ||
| && !(std::is_same_v<type, unsigned char> || std::is_same_v<type, signed char>) |
There was a problem hiding this comment.
| && !(std::is_same_v<type, unsigned char> || std::is_same_v<type, signed char>) | |
| && !(std::is_same_v<type, uint8_t> || std::is_same_v<type, int8_t>) |
This should work i think
There was a problem hiding this comment.
So it will evaluate to false for int8 and uint8 and would try to generate real random numbers.
This requires some more changes.
|
This change puts restriction on what data types random uniform operator can support and should better be checked at |
| @@ -75,7 +75,16 @@ struct random_uniform | |||
|
|
|||
| result.visit([&](auto output) { | |||
| using type = typename decltype(output)::value_type; | |||
| if constexpr(std::is_integral<type>{}) | |||
| if constexpr( | |||
| std::is_integral_v<type> | |||
There was a problem hiding this comment.
Should be std::is_integral<type>{}.
There was a problem hiding this comment.
Why do we need to instantiate the static check? That is a compile-time property check from the static-if and not a runtime check. std::is_integral_v<> evaluates to std::integral<>::value, which is true for integral types and false for others. Honestly, I have not seen someone instantiating static checks.
There was a problem hiding this comment.
Why do we need to instantiate the static check?
You are instantiating the template in both cases.
- By constructing it we get an
integral_constantinstead of bool which is more powerful in some cases(such as passing this to an intermediate function). - Not all type traits provide variable templates, so we need to write it like that
- Consistency. We shouldnt need to think about whether we will pass it to a function or whether it supports a variable template. We write it the same way everytime.
| // for the generator is not one of short, int, long, long long, unsigned short, | ||
| // unsigned int, unsigned long, or unsigned long long. See | ||
| // https://en.cppreference.com/w/cpp/numeric/random/uniform_int_distribution. | ||
| && !(std::is_same_v<type, unsigned char> || std::is_same_v<type, signed char>) |
There was a problem hiding this comment.
This will produce wrong results for int8 on windows. When we are using an unsupported type then we need to use a supported one and then clamp it to the supported:
template<class T>
using uniform_int_distribution = std::uniform_int_distribution<std::conditional_t<sizeof(T) == 1, int, T>>;And then clamp it when we construct it:
uniform_int_distribution<type> dis(0, std::numeric_limits<type>::max());
std::generate(output.begin(), output.end(), [&] { return dis(gen); });There was a problem hiding this comment.
That make sense. I will try it and let you know the result.
There was a problem hiding this comment.
Then we need to use a supported one and then clamp it to the supported:
That would violate how a random uniform should work. It should produce uniformly distributed numbers across the range.
I think we can just not support it for int8 for now. ONNX only supports for real values.
or otherwise we would need to scale it down to range of int8 and then quantize/round it to nearest int
There was a problem hiding this comment.
That would violate how a random uniform should work. It should produce uniformly distributed numbers across the range.
Yea "clamping" was probably a poor choice of words.
or otherwise we would need to scale it down to range of int8 and then quantize/round it to nearest int
We dont need to do that, we can pass the range we want to the constructor. This should produce the same results.
Originally we were relying on the default constructor to set the range based on the data type, but since we are using a different generator type then we need to pass the range to the constructor.
There was a problem hiding this comment.
I strongly recommend not trying to support anything not already supported by STL or other libraries. There will be a mess of special and edge cases to consider, and any shortcoming will create subtle statistical biases that could turn into problems for our future customers at runtime. Coding statistical library functions is a specialized field that we do not want to dabble in.
There was a problem hiding this comment.
A little more reading in StackOverflow indicates that the C++ committee never gave a mathematical reason for disallowing int8 types for uniform_int_distribution. So it could be implemented by instantiating uniform_int_distribution with range arguments(0, 255) in the contructor and then assigning to the desired type.
This build is OK for merge ✅ |
🔴bert_base_cased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output🔴distilgpt2_fp16: FAILED: MIGraphX is not within tolerance - check verbose output |
No, our reference implementation should support all types. |
| // for the generator is not one of short, int, long, long long, unsigned short, | ||
| // unsigned int, unsigned long, or unsigned long long. See | ||
| // https://en.cppreference.com/w/cpp/numeric/random/uniform_int_distribution. | ||
| if constexpr(std::is_same_v<type, unsigned char> || std::is_same_v<type, signed char>) |
There was a problem hiding this comment.
This could be written as std::is_integral<type>{} and sizeof(type) == 1, which I think is simpler.
If not, then it should be std::is_same<type, unsigned char>{} or std::is_same<type, signed char>{} to match the style used throughout migraphx.
See the comment in the header file and https://en.cppreference.com/w/cpp/numeric/random/uniform_int_distribution.
The standard library from Microsoft Visual C++ is strict about that - it fails compilation when
unsigned charandsigned charare used as deduced type/template parameters.The Clang compiler on Windows uses the standard library from MSFT Visual C++.