webnn: Introduce MLNumber [chromium/src : main]
Lisha Guo (Gerrit)
Lisha Guo added 1 comment![]( "Open in Gerrit")
This CL is based on another CL- https://chromium-review.googlesource.com/c/chromium/src/+/5466806 from @asu...@chromium.org. This new CL fixed the remaining issues with the bindings code interpreting numbers as BigInts and added Clamp MLNumber supporting for different backends.
Related details
- ningxin hu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ningxin hu (Gerrit)
ningxin hu added 1 comment![]( "Open in Gerrit")
This CL is based on another CL- https://chromium-review.googlesource.com/c/chromium/src/+/5466806 from @asu...@chromium.org. This new CL fixed the remaining issues with the bindings code interpreting numbers as BigInts and added Clamp MLNumber supporting for different backends.
- Austin Sullivan
- Lisha Guo
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Austin Sullivan (Gerrit)
Austin Sullivan added 2 comments![]( "Open in Gerrit")
What are the high-level diffs between this CL and https://crrev.com/c/5466806?
[FAIL] minValue as Infinity
promise_test: Unhandled rejection with value: object "UnknownError: Failed to execute 'build' on 'MLGraphBuilder': Model compilation error."Hmm ideally we shouldn't be running into model compilation errors. Do you know what's causing this?
Related details
- Lisha Guo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Austin Sullivan (Gerrit)
Austin Sullivan added 1 comment![]( "Open in Gerrit")
assert_less_than_equal: assert_array_approx_equals_ulp: test clamp int64 actual 9223372036854775807 should be close enough to expected 9223372036854776000 by ULP distance: expected a number less than or equal to 0n but got 1nHmm why is this value expected? Is the expected value being implicitly casted somewhere?
Also should we be using ULP to compare integers?
(same thing with 18446744073709552000 below)
Austin Sullivan (Gerrit)
Austin Sullivan added 3 comments![]( "Open in Gerrit")
Took a look and overall everything seems reasonable other than the few comments I've already left. This is a big CL but I understand that it will be hard to meaningfully split up. If we add more unit test coverage then we could try to merge e.g. the BigInt changes separately? Not sure whether splitting just that off is worth the effort, and the other code is all quite intertwined unfortunately
class COMPONENT_EXPORT(WEBNN_PUBLIC_CPP) MLNumber {Let's consider adding unit tests for this class? Most things here are straightforward but it would be nice to have some coverage for the non-trivial methods like `IsGreaterThan` and handling of infinities
// Best-effort cast to a double. This is generally not recommended.
double ToDoubleLossy() const;Let's add unit tests for this class, too? Especially for this method
Lisha Guo (Gerrit)
Lisha Guo added 5 comments![]( "Open in Gerrit")
What are the high-level diffs between this CL and https://crrev.com/c/5466806?
The main diff lies in the implementation of BigInt::Toxxxx of bigInt.cc. Besides, I also added MLNumber support for clamp for CoreML and DirectML.
class COMPONENT_EXPORT(WEBNN_PUBLIC_CPP) MLNumber {Let's consider adding unit tests for this class? Most things here are straightforward but it would be nice to have some coverage for the non-trivial methods like `IsGreaterThan` and handling of infinities
Sure. I will add related unit tests for the non-trivial methods.
// Best-effort cast to a double. This is generally not recommended.
double ToDoubleLossy() const;Let's add unit tests for this class, too? Especially for this method
Sure. I will add related unit tests.
[FAIL] minValue as Infinity
promise_test: Unhandled rejection with value: object "UnknownError: Failed to execute 'build' on 'MLGraphBuilder': Model compilation error."Lisha GuoHmm ideally we shouldn't be running into model compilation errors. Do you know what's causing this?
I also see these model compilation errors, and it only appears on macOS. Since I don't have a Mac machine at hand, I haven't found the reason yet.
assert_less_than_equal: assert_array_approx_equals_ulp: test clamp int64 actual 9223372036854775807 should be close enough to expected 9223372036854776000 by ULP distance: expected a number less than or equal to 0n but got 1nHmm why is this value expected? Is the expected value being implicitly casted somewhere?
Also should we be using ULP to compare integers?
(same thing with 18446744073709552000 below)
Also should we be using ULP to compare integers?
Here, I set ULP = 0 for integers compare.
Is the expected value being implicitly casted somewhere?
I am not sure whether is caused by int64/uint64 precision issue of DirectML clamp op.
Related details
- Austin Sullivan
Austin Sullivan (Gerrit)
Austin Sullivan added 1 comment![]( "Open in Gerrit")
assert_less_than_equal: assert_array_approx_equals_ulp: test clamp int64 actual 9223372036854775807 should be close enough to expected 9223372036854776000 by ULP distance: expected a number less than or equal to 0n but got 1nLisha GuoHmm why is this value expected? Is the expected value being implicitly casted somewhere?
Also should we be using ULP to compare integers?
(same thing with 18446744073709552000 below)
Also should we be using ULP to compare integers?
Here, I set ULP = 0 for integers compare.
Is the expected value being implicitly casted somewhere?
I am not sure whether is caused by int64/uint64 precision issue of DirectML clamp op.
Just to clarify, my comment is about the _expected_ value seeming off - which might indicate an issue with our testing code somewhere. The `actual 9223372036854775807` matches the value of `"expectedOutputs"` in the test file
Related details
- Lisha Guo
Lisha Guo (Gerrit)
Lisha Guo added 1 comment![]( "Open in Gerrit")
[FAIL] minValue as Infinity
promise_test: Unhandled rejection with value: object "UnknownError: Failed to execute 'build' on 'MLGraphBuilder': Model compilation error."Hmm ideally we shouldn't be running into model compilation errors. Do you know what's causing this?
I also see these model compilation errors, and it only appears on macOS. Since I don't have a Mac machine at hand, I haven't found the reason yet.
All these fail test cases with model compilation errors all set the same minValue and maxValue. It seems CoreML can't support same minValue and maxValue for clamp.
Related details
- Austin Sullivan
Lisha Guo (Gerrit)
Lisha Guo added 1 comment![]( "Open in Gerrit")
[FAIL] minValue as Infinity
promise_test: Unhandled rejection with value: object "UnknownError: Failed to execute 'build' on 'MLGraphBuilder': Model compilation error."Lisha GuoHmm ideally we shouldn't be running into model compilation errors. Do you know what's causing this?
Lisha GuoI also see these model compilation errors, and it only appears on macOS. Since I don't have a Mac machine at hand, I haven't found the reason yet.
All these fail test cases with model compilation errors all set the same minValue and maxValue. It seems CoreML can't support same minValue and maxValue for clamp.
We can use `min` and `max` to emulation for these case. @phi...@chromium.org WDYT?
Related details
- Austin Sullivan
- Phillis Tang
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Phillis Tang (Gerrit)
Phillis Tang added 1 comment![]( "Open in Gerrit")
[FAIL] minValue as Infinity
promise_test: Unhandled rejection with value: object "UnknownError: Failed to execute 'build' on 'MLGraphBuilder': Model compilation error."Lisha GuoHmm ideally we shouldn't be running into model compilation errors. Do you know what's causing this?
Lisha GuoI also see these model compilation errors, and it only appears on macOS. Since I don't have a Mac machine at hand, I haven't found the reason yet.
Lisha GuoAll these fail test cases with model compilation errors all set the same minValue and maxValue. It seems CoreML can't support same minValue and maxValue for clamp.
We can use `min` and `max` to emulation for these case. @phi...@chromium.org WDYT?
- Austin Sullivan
- Lisha Guo
Austin Sullivan (Gerrit)
Austin Sullivan added 4 comments![]( "Open in Gerrit")
Lisha GuoWhat are the high-level diffs between this CL and https://crrev.com/c/5466806?
The main diff lies in the implementation of BigInt::Toxxxx of bigInt.cc. Besides, I also added MLNumber support for clamp for CoreML and DirectML.
Ack thanks for the context
} else {This `else` block can be un-nested because all code paths in the preceding `if` block `return`
!base::IsValueInRangeForNumericType<uint8_t>(words_[0])) {Hmm why is is that we're only checking this for the unsigned types?
Related details
- Lisha Guo
Lisha Guo (Gerrit)
Lisha Guo added 2 comments![]( "Open in Gerrit")
} else {This `else` block can be un-nested because all code paths in the preceding `if` block `return`
Good catch. I will remove `else`.
!base::IsValueInRangeForNumericType<uint8_t>(words_[0])) {Hmm why is is that we're only checking this for the unsigned types?
For signed types, we need to check the valid range according to 'sign_bits_`. For example,
```
if (sign_bit_) {
if (value >
static_cast<uint64_t>(std::abs(std::numeric_limits<int8_t>::min()))) {
return std::nullopt;
}
} else {
if (value > static_cast<uint64_t>(std::numeric_limits<int8_t>::max())) {
return std::nullopt;
}
```
Am I right?
Related details
- Austin Sullivan
Lisha Guo (Gerrit)
Lisha Guo added 1 comment![]( "Open in Gerrit")
[FAIL] minValue as Infinity
promise_test: Unhandled rejection with value: object "UnknownError: Failed to execute 'build' on 'MLGraphBuilder': Model compilation error."Hmm ideally we shouldn't be running into model compilation errors. Do you know what's causing this?
Lisha GuoI also see these model compilation errors, and it only appears on macOS. Since I don't have a Mac machine at hand, I haven't found the reason yet.
Lisha GuoAll these fail test cases with model compilation errors all set the same minValue and maxValue. It seems CoreML can't support same minValue and maxValue for clamp.
Phillis TangWe can use `min` and `max` to emulation for these case. @phi...@chromium.org WDYT?
SG, you can create a crbug for that.
related [issue](https://issues.chromium.org/issues/421927615) has been created. And I suppose we can create another new CL to solve this issue.
Related details
- Austin Sullivan
- Phillis Tang
Lisha Guo (Gerrit)
Lisha Guo added 3 comments![]( "Open in Gerrit")
if (sign_bit_) {Lisha Guonit: use `IsNegative()`
Done
Lisha GuoThis `else` block can be un-nested because all code paths in the preceding `if` block `return`
Good catch. I will remove `else`.
Done
assert_less_than_equal: assert_array_approx_equals_ulp: test clamp int64 actual 9223372036854775807 should be close enough to expected 9223372036854776000 by ULP distance: expected a number less than or equal to 0n but got 1nHmm why is this value expected? Is the expected value being implicitly casted somewhere?
Also should we be using ULP to compare integers?
(same thing with 18446744073709552000 below)
Austin SullivanAlso should we be using ULP to compare integers?
Here, I set ULP = 0 for integers compare.
Is the expected value being implicitly casted somewhere?
I am not sure whether is caused by int64/uint64 precision issue of DirectML clamp op.
Just to clarify, my comment is about the _expected_ value seeming off - which might indicate an issue with our testing code somewhere. The `actual 9223372036854775807` matches the value of `"expectedOutputs"` in the test file
For JS, we use `Float64Array` to store expected data for default. The actual data `9223372036854775807` is large than the largest data `9223372036854776000` it can hold. To solve this accuracy loss, we can use `9223372036854775807n` instead of `9223372036854775807` for expected data. WDYT?
Related details
- Austin Sullivan
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Austin Sullivan (Gerrit)
Austin Sullivan added 3 comments![]( "Open in Gerrit")
!base::IsValueInRangeForNumericType<uint8_t>(words_[0])) {Hmm why is is that we're only checking this for the unsigned types?
For signed types, we need to check the valid range according to 'sign_bits_`. For example,
```
if (sign_bit_) {
if (value >
static_cast<uint64_t>(std::abs(std::numeric_limits<int8_t>::min()))) {
return std::nullopt;
}
} else {
if (value > static_cast<uint64_t>(std::numeric_limits<int8_t>::max())) {
return std::nullopt;
}
```Am I right?
Ah right. Thoughts on pulling this out into a templated helper? Something like this:
```
template <typename DstT>
requires(std::is_signed_v<DstT> && std::is_integral_v<DstT>)
std::optional<DstT> GetValueIfInRange(uint64_t value, bool is_negative) {
if (is_negative) {
if (value >
static_cast<uint64_t>(std::abs(std::numeric_limits<DstT>::min()))) {
return std::nullopt;
}
return -static_cast<DstT>(value);
}
if (value > static_cast<uint64_t>(std::numeric_limits<DstT>::max())) {
return std::nullopt;
}
return static_cast<DstT>(value);
}
```[FAIL] minValue as Infinity
promise_test: Unhandled rejection with value: object "UnknownError: Failed to execute 'build' on 'MLGraphBuilder': Model compilation error."Hmm ideally we shouldn't be running into model compilation errors. Do you know what's causing this?
Lisha GuoI also see these model compilation errors, and it only appears on macOS. Since I don't have a Mac machine at hand, I haven't found the reason yet.
Lisha GuoAll these fail test cases with model compilation errors all set the same minValue and maxValue. It seems CoreML can't support same minValue and maxValue for clamp.
Phillis TangWe can use `min` and `max` to emulation for these case. @phi...@chromium.org WDYT?
Lisha GuoSG, you can create a crbug for that.
related [issue](https://issues.chromium.org/issues/421927615) has been created. And I suppose we can create another new CL to solve this issue.
Ack. Let's tag that bug to the CL footer?
assert_less_than_equal: assert_array_approx_equals_ulp: test clamp int64 actual 9223372036854775807 should be close enough to expected 9223372036854776000 by ULP distance: expected a number less than or equal to 0n but got 1nLisha GuoHmm why is this value expected? Is the expected value being implicitly casted somewhere?
Also should we be using ULP to compare integers?
(same thing with 18446744073709552000 below)
Austin SullivanAlso should we be using ULP to compare integers?
Here, I set ULP = 0 for integers compare.
Is the expected value being implicitly casted somewhere?
I am not sure whether is caused by int64/uint64 precision issue of DirectML clamp op.
Lisha GuoJust to clarify, my comment is about the _expected_ value seeming off - which might indicate an issue with our testing code somewhere. The `actual 9223372036854775807` matches the value of `"expectedOutputs"` in the test file
For JS, we use `Float64Array` to store expected data for default. The actual data `9223372036854775807` is large than the largest data `9223372036854776000` it can hold. To solve this accuracy loss, we can use `9223372036854775807n` instead of `9223372036854775807` for expected data. WDYT?
- Lisha Guo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dwayne Robinson (Gerrit)
Dwayne Robinson added 9 comments![]( "Open in Gerrit")
Thanks for resurrecting this old change Lisha.
switch (output_tensor_desc.GetDataType()) {`ToScalarUnion` already exists - do we need this separate `switch` statement?
```c++
auto data_type = output_tensor_desc.GetDataType();
clamp_operator_desc.Min = ToScalarUnion(clamp->min_value, data_type);
clamp_operator_desc.Max = ToScalarUnion(clamp->max_value, data_type);
```
const float min_value = clamp.min_value.AsFloat32();(already existing - can open separate Chromium issue for it)
Won't coercing to float lose precision for integers? 16777217 would be treated as if 16777216. I see `tfl.minimum` supports u/int32 and u/int64 [here](https://www.tensorflow.org/mlir/tfl_ops#tflminimum_tflminimumop), and so integers *should* be supportable too.
return (!clamp.min_value.IsGreaterThan(clamp.max_value, data_type));```suggestion
return !clamp.min_value.IsGreaterThan(clamp.max_value, data_type);
```
LOG(ERROR) << "bigint.ToInt64(): " << *int64;Why is this case an error? 🤔 Is this debugging litter?
// 'data': [295n, 127n, 5n, 0n],Meant to delete?
'name': 'minValue as BitInt == maxValue as BigInt',`BigInt`
{'input': 'clampInput'}, {'options': {'minValue': 1, 'maxValue': 1n}}`1n`? If `1` is intended for a heterogeneous case, then I propose updating the test case title just to be clear:
`minValue as Number == maxValue as BigInt`
Lisha Guo (Gerrit)
Lisha Guo added 10 comments![]( "Open in Gerrit")
switch (output_tensor_desc.GetDataType()) {`ToScalarUnion` already exists - do we need this separate `switch` statement?
```c++
auto data_type = output_tensor_desc.GetDataType();
clamp_operator_desc.Min = ToScalarUnion(clamp->min_value, data_type);
clamp_operator_desc.Max = ToScalarUnion(clamp->max_value, data_type);
```
Hmm, when FL < 5.0, I suppose `clamp_operator_desc.Min` can't accept a `DML_SCALAR_UNION`.
const float min_value = clamp.min_value.AsFloat32();(already existing - can open separate Chromium issue for it)
Won't coercing to float lose precision for integers? 16777217 would be treated as if 16777216. I see `tfl.minimum` supports u/int32 and u/int64 [here](https://www.tensorflow.org/mlir/tfl_ops#tflminimum_tflminimumop), and so integers *should* be supportable too.
Left a TODO here.
return (!clamp.min_value.IsGreaterThan(clamp.max_value, data_type));Lisha Guo```suggestion
return !clamp.min_value.IsGreaterThan(clamp.max_value, data_type);
```
Done
Why is this case an error? 🤔 Is this debugging litter?
Good catch. Removed this debugging litter.
// 'data': [295n, 127n, 5n, 0n],Lisha GuoMeant to delete?
Done
'name': 'minValue as BitInt == maxValue as BigInt',Lisha Guo`BigInt`
Done
{'input': 'clampInput'}, {'options': {'minValue': 1, 'maxValue': 1n}}`1n`? If `1` is intended for a heterogeneous case, then I propose updating the test case title just to be clear:
`minValue as Number == maxValue as BigInt`
Done
[FAIL] minValue as Infinity
promise_test: Unhandled rejection with value: object "UnknownError: Failed to execute 'build' on 'MLGraphBuilder': Model compilation error."Lisha GuoHmm ideally we shouldn't be running into model compilation errors. Do you know what's causing this?
Lisha GuoI also see these model compilation errors, and it only appears on macOS. Since I don't have a Mac machine at hand, I haven't found the reason yet.
Lisha GuoAll these fail test cases with model compilation errors all set the same minValue and maxValue. It seems CoreML can't support same minValue and maxValue for clamp.
Phillis TangWe can use `min` and `max` to emulation for these case. @phi...@chromium.org WDYT?
Lisha GuoSG, you can create a crbug for that.
Austin Sullivanrelated [issue](https://issues.chromium.org/issues/421927615) has been created. And I suppose we can create another new CL to solve this issue.
Ack. Let's tag that bug to the CL footer?
Ack. Let's tag that bug to the CL footer?
How about leave a TODO in `graph_builder_coreml.cc` file instead of tagging this bug to the CL footer since I intend to solve this bug in another CL?
assert_less_than_equal: assert_array_approx_equals_ulp: test clamp int64 actual 9223372036854775807 should be close enough to expected 9223372036854776000 by ULP distance: expected a number less than or equal to 0n but got 1nLisha GuoHmm why is this value expected? Is the expected value being implicitly casted somewhere?
Also should we be using ULP to compare integers?
(same thing with 18446744073709552000 below)
Austin SullivanAlso should we be using ULP to compare integers?
Here, I set ULP = 0 for integers compare.
Is the expected value being implicitly casted somewhere?
I am not sure whether is caused by int64/uint64 precision issue of DirectML clamp op.
Lisha GuoJust to clarify, my comment is about the _expected_ value seeming off - which might indicate an issue with our testing code somewhere. The `actual 9223372036854775807` matches the value of `"expectedOutputs"` in the test file
Austin SullivanFor JS, we use `Float64Array` to store expected data for default. The actual data `9223372036854775807` is large than the largest data `9223372036854776000` it can hold. To solve this accuracy loss, we can use `9223372036854775807n` instead of `9223372036854775807` for expected data. WDYT?
SGTM thanks
(resolve once completed)
Since these two test cases both passed in latest CL, resolve this issue.
Related details
- Austin Sullivan
- Dwayne Robinson
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Austin Sullivan (Gerrit)
Austin Sullivan added 1 comment![]( "Open in Gerrit")
[FAIL] minValue as Infinity
promise_test: Unhandled rejection with value: object "UnknownError: Failed to execute 'build' on 'MLGraphBuilder': Model compilation error."Lisha GuoHmm ideally we shouldn't be running into model compilation errors. Do you know what's causing this?
Lisha GuoI also see these model compilation errors, and it only appears on macOS. Since I don't have a Mac machine at hand, I haven't found the reason yet.
Lisha GuoAll these fail test cases with model compilation errors all set the same minValue and maxValue. It seems CoreML can't support same minValue and maxValue for clamp.
Phillis TangWe can use `min` and `max` to emulation for these case. @phi...@chromium.org WDYT?
Lisha GuoSG, you can create a crbug for that.
Austin Sullivanrelated [issue](https://issues.chromium.org/issues/421927615) has been created. And I suppose we can create another new CL to solve this issue.
Lisha GuoAck. Let's tag that bug to the CL footer?
Ack. Let's tag that bug to the CL footer?
How about leave a TODO in `graph_builder_coreml.cc` file instead of tagging this bug to the CL footer since I intend to solve this bug in another CL?
- Dwayne Robinson
- Lisha Guo
Dwayne Robinson (Gerrit)
Dwayne Robinson added 2 comments![]( "Open in Gerrit")
switch (output_tensor_desc.GetDataType()) {Lisha Guo`ToScalarUnion` already exists - do we need this separate `switch` statement?
```c++
auto data_type = output_tensor_desc.GetDataType();
clamp_operator_desc.Min = ToScalarUnion(clamp->min_value, data_type);
clamp_operator_desc.Max = ToScalarUnion(clamp->max_value, data_type);
```
Hmm, when FL < 5.0, I suppose `clamp_operator_desc.Min` can't accept a `DML_SCALAR_UNION`.
Doh, yes, lines 1710 and 1713 already handle the `DML_FEATURE_LEVEL_5_0` case.
Related details
- Lisha Guo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lisha Guo (Gerrit)
Lisha Guo added 3 comments![]( "Open in Gerrit")
class COMPONENT_EXPORT(WEBNN_PUBLIC_CPP) MLNumber {Lisha GuoLet's consider adding unit tests for this class? Most things here are straightforward but it would be nice to have some coverage for the non-trivial methods like `IsGreaterThan` and handling of infinities
Sure. I will add related unit tests for the non-trivial methods.
Added `ml_number_unittest.cc` to cover more methods. PTAL. Thanks!
!base::IsValueInRangeForNumericType<uint8_t>(words_[0])) {Hmm why is is that we're only checking this for the unsigned types?
Austin SullivanFor signed types, we need to check the valid range according to 'sign_bits_`. For example,
```
if (sign_bit_) {
if (value >
static_cast<uint64_t>(std::abs(std::numeric_limits<int8_t>::min()))) {
return std::nullopt;
}
} else {
if (value > static_cast<uint64_t>(std::numeric_limits<int8_t>::max())) {
return std::nullopt;
}
```Am I right?
Ah right. Thoughts on pulling this out into a templated helper? Something like this:
```
template <typename DstT>
requires(std::is_signed_v<DstT> && std::is_integral_v<DstT>)
std::optional<DstT> GetValueIfInRange(uint64_t value, bool is_negative) {
if (is_negative) {
if (value >
static_cast<uint64_t>(std::abs(std::numeric_limits<DstT>::min()))) {
return std::nullopt;
}
return -static_cast<DstT>(value);
}if (value > static_cast<uint64_t>(std::numeric_limits<DstT>::max())) {
return std::nullopt;
}
return static_cast<DstT>(value);
}
```
Done
[FAIL] minValue as Infinity
promise_test: Unhandled rejection with value: object "UnknownError: Failed to execute 'build' on 'MLGraphBuilder': Model compilation error."Lisha GuoHmm ideally we shouldn't be running into model compilation errors. Do you know what's causing this?
Lisha GuoI also see these model compilation errors, and it only appears on macOS. Since I don't have a Mac machine at hand, I haven't found the reason yet.
Lisha GuoAll these fail test cases with model compilation errors all set the same minValue and maxValue. It seems CoreML can't support same minValue and maxValue for clamp.
Phillis TangWe can use `min` and `max` to emulation for these case. @phi...@chromium.org WDYT?
Lisha GuoSG, you can create a crbug for that.
Austin Sullivanrelated [issue](https://issues.chromium.org/issues/421927615) has been created. And I suppose we can create another new CL to solve this issue.
Lisha GuoAck. Let's tag that bug to the CL footer?
Austin SullivanAck. Let's tag that bug to the CL footer?
How about leave a TODO in `graph_builder_coreml.cc` file instead of tagging this bug to the CL footer since I intend to solve this bug in another CL?
Sure, that works too
- Austin Sullivan
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Austin Sullivan (Gerrit)
Austin Sullivan added 3 comments![]( "Open in Gerrit")
class COMPONENT_EXPORT(WEBNN_PUBLIC_CPP) MLNumber {Lisha GuoLet's consider adding unit tests for this class? Most things here are straightforward but it would be nice to have some coverage for the non-trivial methods like `IsGreaterThan` and handling of infinities
Lisha GuoSure. I will add related unit tests for the non-trivial methods.
Added `ml_number_unittest.cc` to cover more methods. PTAL. Thanks!
LGTM % a couple comments. Thanks!
class MLNumberTest : public testing::Test {};Since this class isn't doing anything, you could alternatively remove it and use `TEST` instead of `TEST_F`
TEST_F(MLNumberTest, IsGreaterThanForFloat16) {
MLNumber a = MLNumber::FromFloat16(12324);
MLNumber b = MLNumber::FromFloat16(-3421);
EXPECT_TRUE(a.IsGreaterThan(b, OperandDataType::kFloat16));
EXPECT_FALSE(b.IsGreaterThan(a, OperandDataType::kFloat16));
}Let's also test float vs int comparisions?
Related details
- Lisha Guo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lisha Guo (Gerrit)
Lisha Guo added 1 comment![]( "Open in Gerrit")
// Best-effort cast to a double. This is generally not recommended.
double ToDoubleLossy() const;Lisha GuoLet's add unit tests for this class, too? Especially for this method
Lisha GuoSure. I will add related unit tests.
As I understand, if we want to add unit tests here for bigint, we need to construct a `bigint` firstly something like below
```
int64_t data = 123;
v8::Local<v8::BigInt> v8_bigint = v8::BigInt::New(isolate, data);
BigInt bigint(v8_bigint);
```
When try to construct a V8 object, we need a `v8::isolate`. For `blink_platform_unittests`, I tried several way to get `v8::isolate`. However, they all failed.
```
V8TestingScope scope;
v8::Isolate* isolate = scope.GetIsolate();
```
or
```
test::TaskEnvironment task_environment_;
v8::Isolate* isolate = task_environment_.isolate();
```
It seems `blink_platform_unittests` lacks of v8 enviroment to construct a v8 object. But it's easy to create for `blink_unittests`. I also tried to load V8 using `gin::V8Initializer::LoadV8Snapshot(kSnapshotType);` it also failed.
@asu...@chromium.org Did I miss something important for `blink_platform_unittests`.
Related details
- Austin Sullivan
ningxin hu (Gerrit)
ningxin hu added 6 comments![]( "Open in Gerrit")
case webnn::OperandDataType::kFloat32:
// Implicit double -> float conversion.
return webnn::MLNumber::FromFloat32(bigint.ToDoubleLossy());
case webnn::OperandDataType::kFloat16:
// double (-> float, implicitly) -> fp16 (as uint16_t).
return webnn::MLNumber::FromFloat16(
fp16_ieee_from_fp32_value(bigint.ToDoubleLossy()));
case webnn::OperandDataType::kInt32: {
if (auto int32 = bigint.ToInt32()) {
return webnn::MLNumber::FromInt32(*int32);
} else {
return webnn::MLNumber::FromInt32(
bigint.IsNegative() ? std::numeric_limits<int32_t>::min()
: std::numeric_limits<int32_t>::max());
}
}
case webnn::OperandDataType::kUint32: {
if (auto uint32 = bigint.ToUInt32()) {
return webnn::MLNumber::FromUint32(*uint32);
} else {
return webnn::MLNumber::FromUint32(
bigint.IsNegative() ? std::numeric_limits<uint32_t>::min()
: std::numeric_limits<uint32_t>::max());
}
}Should we report an error? I suppose user code should only use BigInt for "int64" and "uint64" data types.
case webnn::OperandDataType::kInt64:
return webnn::MLNumber::FromInt64(
base::saturated_cast<int64_t>(unrestricted_double));
case webnn::OperandDataType::kUint64:
return webnn::MLNumber::FromUint64(
base::saturated_cast<uint64_t>(unrestricted_double));Same here, should we report an error? Number should not be used for "int64" and "uint64" data types?
const double unrestricted_double = number.GetAsUnrestrictedDouble();
switch (type) {
case webnn::OperandDataType::kFloat32:
// Implicit double -> float conversion.
return webnn::MLNumber::FromFloat32(unrestricted_double);
case webnn::OperandDataType::kFloat16:
// double (-> float, implicitly) -> fp16 (as uint16_t).
return webnn::MLNumber::FromFloat16(
fp16_ieee_from_fp32_value(unrestricted_double));
case webnn::OperandDataType::kInt32:
return webnn::MLNumber::FromInt32(
base::saturated_cast<int32_t>(unrestricted_double));
case webnn::OperandDataType::kUint32:
return webnn::MLNumber::FromUint32(
base::saturated_cast<uint32_t>(unrestricted_double));
case webnn::OperandDataType::kInt64:
return webnn::MLNumber::FromInt64(
base::saturated_cast<int64_t>(unrestricted_double));
case webnn::OperandDataType::kUint64:
return webnn::MLNumber::FromUint64(
base::saturated_cast<uint64_t>(unrestricted_double));
case webnn::OperandDataType::kInt8:
return webnn::MLNumber::FromInt8(
base::saturated_cast<int8_t>(unrestricted_double));
case webnn::OperandDataType::kUint8:
return webnn::MLNumber::FromUint8(
base::saturated_cast<uint8_t>(unrestricted_double));
case webnn::OperandDataType::kInt4:
case webnn::OperandDataType::kUint4:
NOTREACHED();
}Could we just construct a `webnn::Number` with the `unrestricted_double` value? `webnn::Number` can hold a double and convert to destination data types at the backends. Conversions here seem to be unnecessary. Did I miss anything?
```suggestion
return webnn::MLNumber::FromFloat64(number.GetAsUnrestrictedDouble());
```
pow(18446744073709551616.0, i);Use `std::numeric_limits<uint64_t>::max() + 1`?
Related details
- Austin Sullivan
- Lisha Guo
Dwayne Robinson (Gerrit)
Dwayne Robinson added 2 comments![]( "Open in Gerrit")
case webnn::OperandDataType::kFloat32:
// Implicit double -> float conversion.
return webnn::MLNumber::FromFloat32(bigint.ToDoubleLossy());
case webnn::OperandDataType::kFloat16:
// double (-> float, implicitly) -> fp16 (as uint16_t).
return webnn::MLNumber::FromFloat16(
fp16_ieee_from_fp32_value(bigint.ToDoubleLossy()));
case webnn::OperandDataType::kInt32: {
if (auto int32 = bigint.ToInt32()) {
return webnn::MLNumber::FromInt32(*int32);
} else {
return webnn::MLNumber::FromInt32(
bigint.IsNegative() ? std::numeric_limits<int32_t>::min()
: std::numeric_limits<int32_t>::max());
}
}
case webnn::OperandDataType::kUint32: {
if (auto uint32 = bigint.ToUInt32()) {
return webnn::MLNumber::FromUint32(*uint32);
} else {
return webnn::MLNumber::FromUint32(
bigint.IsNegative() ? std::numeric_limits<uint32_t>::min()
: std::numeric_limits<uint32_t>::max());
}
}Should we report an error? I suppose user code should only use BigInt for "int64" and "uint64" data types.
This is great that a user can easily pass an ordinary Number for int64 tensors (which helps generic caller code regardless of the tensor type, such as a lower bound to `clamp`), instead of needing to clumsily work-around that awkward faux pas of Javascript where int64 usage needs to use a completely different and incompatible type from all other JS math.
It's robust that all this saturation logic is consolidated in one place, rather than each backend needing to gingerly consistently replicate it (subject to their own truncation logic).
ningxin hu (Gerrit)
ningxin hu added 2 comments![]( "Open in Gerrit")
needing to clumsily work-around that awkward faux pas of Javascript where int64 usage needs to use a completely different and incompatible type from all other JS math.
I suppose this is JavaScript convention. For example you have to use BigInt for BigInt64Array/BigUint64Array. For all other typed arrays, you just use Number.
Because MLNumber is just a union of BigInt and Number, it would be good that user can specify its usage explicitly: BigInt for "int64" and "uint64", Number for all the others. We also check the type of typed array when passing it to `MLGraphBuilder.constant()`.
It's robust that all this saturation logic is consolidated in one place
I agreed. I meant backends can use `webnn::MLNumber::As<DstType>()`. Because the `webnn::Number` actually carries the `std::variant<double, int64_t, uint64_t> number_`, I suppose conversion during the construction is unnecessary.
Dwayne Robinson (Gerrit)
Dwayne Robinson added 3 comments![]( "Open in Gerrit")
Consider the following generic caller code, which returns a new tensor (be it int32, int64, float32...) with the bounds between 0 and 6.
```js
function emulateRelu6(graphBuilder, inputTensor)
{
return graphBuilder.clamp(inputTensor, {minValue: 0, maxValue: 6});
}
function multiplyByTwo(graphBuilder, inputTensor)
{
const twoFactor = graphBuilder.constant(inputTensor.dataType, 2);
return graphBuilder.mul(inputTensor, twoFactor);
}
```
With such a requirement imposed, then it would have to be more like...
```js
function emulateRelu6(graphBuilder, inputTensor)
{
bool is64BitDataType = (inputTensor.dataType == "uint64")
|| (inputTensor.dataType == "int64");
return graphBuilder.clamp(
inputTensor,
{
minValue: is64BitDataType ? 0: 0n,
maxValue: is64BitDataType ? 6: 6n
}
);
}
function multiplyByTwo(graphBuilder, inputTensor) ...
```
...which makes WebNN a clumsier API for callers.
Another reason to support `Number` for `minValue` and `maxValue` with int64 tensors is compat. Before, `clamp` took float32 `minValue` and `maxValue`, and generic code would work okay with {int8, int32, **int64**, float32} tensors, and of course, not all possible int64 values were expressible (which is why `MLNumber` was added), but of the subset of int64 values that *were* expressible, you could pass them in generically (like relu6's 0 and 6), and such code would still work after Lisha's change.
The last reason is regularity. Wouldn't it be really weird if say the "int8" data type required a special literal suffix? e.g.:
```js
graphBuilder.constant("int8" , 42c); // <<<<
graphBuilder.constant("int16", 42);
graphBuilder.constant("int32", 42);
graphBuilder.constant("int64", 42);
```
Conversely, if that is weird, then it's also weird that int64 requires a different literal suffix:
```
graphBuilder.constant("int8" , 42);
graphBuilder.constant("int16", 42);
graphBuilder.constant("int32", 42);
graphBuilder.constant("int64", 42n); // <<<<
```
We just might not see it as weird because we've become used to (and blind to) it's aberrance, but it increases the coupling between separate things (the dataType string and its value.
So, I welcome potential simplifications to this conversion function, but from the WebNN caller perspective, this *would* be niceness 😉.
suppose conversion during the construction is unnecessary
True. As long as this saturation functionality is still consolidated and easily callable from each of backends (if a backend wants to coerce from float32 to float32), then I don't mind deferring that saturation until later.
Lisha Guo (Gerrit)
Lisha Guo added 1 comment![]( "Open in Gerrit")
This method is used for comparing `min` and `max` value by `isGreaterThan()` method in `ml_graph_builder.cc` file. Currently, webnn doesn't support float64 data type.
Related details
- Austin Sullivan
- Dwayne Robinson
- ningxin hu
ningxin hu (Gerrit)
ningxin hu added 3 comments![]( "Open in Gerrit")
case webnn::OperandDataType::kFloat32:
// Implicit double -> float conversion.
return webnn::MLNumber::FromFloat32(bigint.ToDoubleLossy());
case webnn::OperandDataType::kFloat16:
// double (-> float, implicitly) -> fp16 (as uint16_t).
return webnn::MLNumber::FromFloat16(
generic code would work okay with {int8, int32, int64, float32} tensors
I agreed. My comment here is more about BigInt usage. I propose we should restrict BigInt is only used for "int64" and "uint64", when users want to set the values larger than Number.MAX_SAFE_INTEGER. I don't think using BigInt for other data types, like "float32", makes much sense.
It would also help to simplify the BigInt modification in this CL, it doesn't require to implement `ToInt8()`, `ToInt32()`, `ToUInt8()`, `ToUInt32()` and `ToDoubleLossy()`.
case webnn::OperandDataType::kInt64:
return webnn::MLNumber::FromInt64(
base::saturated_cast<int64_t>(unrestricted_double));
case webnn::OperandDataType::kUint64:
return webnn::MLNumber::FromUint64(
base::saturated_cast<uint64_t>(unrestricted_double));ningxin huSame here, should we report an error? Number should not be used for "int64" and "uint64" data types?
Resolving this comment, as I agreed with @dwa...@microsoft.com, allowing Number for "int64" and "uint64" would be useful.
`IsGreaterThan()` is a `webnn::MLNumber` method. It convert to destination data type for comparison internally: https://chromium-review.googlesource.com/c/chromium/src/+/6565815/23/services/webnn/public/cpp/ml_number.cc#102
Related details
- Austin Sullivan
- Dwayne Robinson
- Lisha Guo
Dwayne Robinson (Gerrit)
Dwayne Robinson added 2 comments![]( "Open in Gerrit")
👍
I propose we should restrict BigInt is only used for "int64" and "uint64" ... I don't think using BigInt for other data types, like "float32", makes much sense.
Hmm, well it's nice to be generic and symmetric (because someone could pass `BigInt(42n)`, which is perfectly representable as `float32`), but yeah, I don't really care about that case, because if someone *explicitly* passed `BigInt`, then it's quite likely it's because they also passed an int64 tensor anyway. Proposal = 👍.
Related details
- Austin Sullivan
- Lisha Guo
Lisha Guo (Gerrit)
Lisha Guo added 2 comments![]( "Open in Gerrit")
I propose we should restrict BigInt is only used for "int64" and "uint64", when users want to set the values larger than Number.MAX_SAFE_INTEGER.
Sounds good. @asu...@chromium.org WDYT?
const double unrestricted_double = number.GetAsUnrestrictedDouble();
switch (type) {
case webnn::OperandDataType::kFloat32:
// Implicit double -> float conversion.
return webnn::MLNumber::FromFloat32(unrestricted_double);
case webnn::OperandDataType::kFloat16:
// double (-> float, implicitly) -> fp16 (as uint16_t).
return webnn::MLNumber::FromFloat16(
It convert to destination data type for comparison internally...
You are right. Agree to just return `webnn::MLNumber::FromFloat64(number.GetAsUnrestrictedDouble());` for double.
Related details
- Austin Sullivan
- Dwayne Robinson
- ningxin hu
Austin Sullivan (Gerrit)
Austin Sullivan added 3 comments![]( "Open in Gerrit")
Apologies for the delay!
I was trying to remember why I had initially decided that we should be able to convert a `BigInt` to any other data type, and I think the primary reason was because at that time `clamp()` was an `MLActivation` and [the data type of an `MLActivation` depended on the operator it was passed to](https://github.com/webmachinelearning/webnn/pull/647#discussion_r1571435646)
But `MLActivation` doesn't exist anymore, so AFAIK we don't _need_ this functionality. You could argue it would be nice to have and I acknowledge the concern from @dwa...@microsoft.com that this may lead to uglier code from callers, but I'd lean towards removing it. Removal would simplify this CL and we could easily add this functionality back later if needed
// Best-effort cast to a double. This is generally not recommended.
double ToDoubleLossy() const;Lisha GuoLet's add unit tests for this class, too? Especially for this method
Lisha GuoSure. I will add related unit tests.
As I understand, if we want to add unit tests here for bigint, we need to construct a `bigint` firstly something like below
```
int64_t data = 123;
v8::Local<v8::BigInt> v8_bigint = v8::BigInt::New(isolate, data);
BigInt bigint(v8_bigint);
```
When try to construct a V8 object, we need a `v8::isolate`. For `blink_platform_unittests`, I tried several way to get `v8::isolate`. However, they all failed.
```
V8TestingScope scope;
v8::Isolate* isolate = scope.GetIsolate();
```
or
```
test::TaskEnvironment task_environment_;
v8::Isolate* isolate = task_environment_.isolate();
```
It seems `blink_platform_unittests` lacks of v8 enviroment to construct a v8 object. But it's easy to create for `blink_unittests`. I also tried to load V8 using `gin::V8Initializer::LoadV8Snapshot(kSnapshotType);` it also failed.
@asu...@chromium.org Did I miss something important for `blink_platform_unittests`.
These tests are no longer needed if we only support `BigInt` for 64-bit ints
Related details
- Dwayne Robinson
- Lisha Guo
- ningxin hu
Lisha Guo (Gerrit)
Lisha Guo added 8 comments![]( "Open in Gerrit")
Since this class isn't doing anything, you could alternatively remove it and use `TEST` instead of `TEST_F`
Done
TEST_F(MLNumberTest, IsGreaterThanForFloat16) {
MLNumber a = MLNumber::FromFloat16(12324);
MLNumber b = MLNumber::FromFloat16(-3421);
EXPECT_TRUE(a.IsGreaterThan(b, OperandDataType::kFloat16));
EXPECT_FALSE(b.IsGreaterThan(a, OperandDataType::kFloat16));
}Let's also test float vs int comparisions?
Done
case webnn::OperandDataType::kFloat32:Restrict BigInt is only used for "int64" and "uint64" in latest CL.
const double unrestricted_double = number.GetAsUnrestrictedDouble();
switch (type) {
case webnn::OperandDataType::kFloat32:
// Implicit double -> float conversion.
return webnn::MLNumber::FromFloat32(unrestricted_double);
case webnn::OperandDataType::kFloat16:
// double (-> float, implicitly) -> fp16 (as uint16_t).
return webnn::MLNumber::FromFloat16(
fp16_ieee_from_fp32_value(unrestricted_double));
case webnn::OperandDataType::kInt32:
return webnn::MLNumber::FromInt32(
base::saturated_cast<int32_t>(unrestricted_double));
case webnn::OperandDataType::kUint32:
return webnn::MLNumber::FromUint32(
base::saturated_cast<uint32_t>(unrestricted_double));
case webnn::OperandDataType::kInt64:
return webnn::MLNumber::FromInt64(
base::saturated_cast<int64_t>(unrestricted_double));
case webnn::OperandDataType::kUint64:
return webnn::MLNumber::FromUint64(
base::saturated_cast<uint64_t>(unrestricted_double));
case webnn::OperandDataType::kInt8:
return webnn::MLNumber::FromInt8(
base::saturated_cast<int8_t>(unrestricted_double));
case webnn::OperandDataType::kUint8:
return webnn::MLNumber::FromUint8(
base::saturated_cast<uint8_t>(unrestricted_double));
case webnn::OperandDataType::kInt4:
case webnn::OperandDataType::kUint4:
NOTREACHED();
}Dwayne RobinsonCould we just construct a `webnn::Number` with the `unrestricted_double` value? `webnn::Number` can hold a double and convert to destination data types at the backends. Conversions here seem to be unnecessary. Did I miss anything?
```suggestion
return webnn::MLNumber::FromFloat64(number.GetAsUnrestrictedDouble());
```
ningxin huIt's robust that all this saturation logic is consolidated in one place, rather than each backend needing to gingerly consistently replicate it (subject to their own truncation logic).
Dwayne RobinsonIt's robust that all this saturation logic is consolidated in one place
I agreed. I meant backends can use `webnn::MLNumber::As<DstType>()`. Because the `webnn::Number` actually carries the `std::variant<double, int64_t, uint64_t> number_`, I suppose conversion during the construction is unnecessary.
Lisha Guosuppose conversion during the construction is unnecessary
True. As long as this saturation functionality is still consolidated and easily callable from each of backends (if a backend wants to coerce from float32 to float32), then I don't mind deferring that saturation until later.
ningxin huThis method is used for comparing `min` and `max` value by `isGreaterThan()` method in `ml_graph_builder.cc` file. Currently, webnn doesn't support float64 data type.
Lisha Guo`IsGreaterThan()` is a `webnn::MLNumber` method. It convert to destination data type for comparison internally: https://chromium-review.googlesource.com/c/chromium/src/+/6565815/23/services/webnn/public/cpp/ml_number.cc#102
It convert to destination data type for comparison internally...You are right. Agree to just return `webnn::MLNumber::FromFloat64(number.GetAsUnrestrictedDouble());` for double.
Done
// Best-effort cast to a double. This is generally not recommended.
double ToDoubleLossy() const;Lisha GuoLet's add unit tests for this class, too? Especially for this method
Lisha GuoSure. I will add related unit tests.
Austin SullivanAs I understand, if we want to add unit tests here for bigint, we need to construct a `bigint` firstly something like below
```
int64_t data = 123;
v8::Local<v8::BigInt> v8_bigint = v8::BigInt::New(isolate, data);
BigInt bigint(v8_bigint);
```
When try to construct a V8 object, we need a `v8::isolate`. For `blink_platform_unittests`, I tried several way to get `v8::isolate`. However, they all failed.
```
V8TestingScope scope;
v8::Isolate* isolate = scope.GetIsolate();
```
or
```
test::TaskEnvironment task_environment_;
v8::Isolate* isolate = task_environment_.isolate();
```
It seems `blink_platform_unittests` lacks of v8 enviroment to construct a v8 object. But it's easy to create for `blink_unittests`. I also tried to load V8 using `gin::V8Initializer::LoadV8Snapshot(kSnapshotType);` it also failed.
@asu...@chromium.org Did I miss something important for `blink_platform_unittests`.
These tests are no longer needed if we only support `BigInt` for 64-bit ints
Done
Use `std::numeric_limits<uint64_t>::max() + 1`?
- Austin Sullivan
- Dwayne Robinson
- ningxin hu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lisha Guo (Gerrit)
Lisha Guo added 1 comment![]( "Open in Gerrit")
@har...@chromium.org Could you please help to review these files? third_party/blink/renderer/bindings/scripts/bind_gen/union.py
third_party/blink/renderer/platform/bindings/bigint.h
third_party/blink/renderer/platform/bindings/bigint.cc. Thanks!
Related details
- Austin Sullivan
- Dwayne Robinson
- Kentaro Hara
- ningxin hu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kentaro Hara (Gerrit)
Kentaro Hara added 1 comment![]( "Open in Gerrit")
yukishiino: Would you take a look at binding changes?
Related details
- Austin Sullivan
- Dwayne Robinson
- Lisha Guo
- Yuki Shiino
- ningxin hu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuki Shiino (Gerrit)
Yuki Shiino added 3 comments![]( "Open in Gerrit")
bindings/ mostly LGTM, but I'd recommend checking whether we can leverage base::CheckedNumeric<T>.
super nit: Better to remove this empty line as it looks inconsistent with other code blocks.
return static_cast<int64_t>(value);Overall, it looks better to use CheckedNumeric<int64_t> if possible, avoiding the reinvention.
I think we can have something like the following code:
```
base::CheckedNumeric<int64_t> value(words_[0]);
if (!value.IsValid()) [[unlikely]] {
return std::nullopt;
}
return value.ValueOrDie();
```
Related details
- Austin Sullivan
- Dwayne Robinson
- Lisha Guo
- ningxin hu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuki Shiino (Gerrit)
Yuki Shiino added 3 comments![]( "Open in Gerrit")
static_cast<uint64_t>(std::abs(std::numeric_limits<int64_t>::min()))) {std::numeric_limits::min() returns a negative value, and in two's complement representation, the absolute value of that negative value cannot be represented in the same type. E.g. in case of signed char, min = -128 and max = 127, and abs(-128) cannot be represented in signed char.
I'm afraid that this code doesn't work as expected. It's nice to have a unittest to prove the behaviors in corner cases (with UB sanitizer).
https://en.cppreference.com/w/cpp/numeric/math/abs
> The behavior is undefined if the result cannot be represented by the return type.
and
In 2's complement systems, the absolute value of the most-negative value is out of range, e.g. for 32-bit 2's complement type int, INT_MIN is -2147483648, but the would-be result 2147483648 is greater than INT_MAX, which is 2147483647.
return -static_cast<int64_t>(value);Probably we can implement this as UB-free based on the followings. We need to be extra-careful. Maybe better to be implemented in //base and reviewed by experts.
C++ 7.8 Integral conversions [conv.integral]
> 2 If the destination type is unsigned, the resulting value is the least unsigned integer congruent to the source integer (modulo 2^n where n is the number of bits used to represent the unsigned type). [ Note: In a two’s complement representation, this conversion is conceptual and there is no change in the bit pattern (if there is no truncation). — end note ]
3 If the destination type is signed, the value is unchanged if it can be represented in the destination type; otherwise, the value is implementation-defined.
5 The conversions allowed as integral promotions are excluded from the set of integral conversions.
C++ 8.5.2.1 Unary operators [expr.unary.op]
> 8 The operand of the unary - operator shall have arithmetic or unscoped enumeration type and the result is the negation of its operand. Integral promotion is performed on integral or enumeration operands. The negative of an unsigned quantity is computed by subtracting its value from 2^n, where n is the number of bits in the promoted operand. The type of the result is the type of the promoted operand.
return static_cast<int64_t>(value);Overall, it looks better to use CheckedNumeric<int64_t> if possible, avoiding the reinvention.
I think we can have something like the following code:
```
base::CheckedNumeric<int64_t> value(words_[0]);
if (!value.IsValid()) [[unlikely]] {
return std::nullopt;
}
return value.ValueOrDie();
```
I overlooked the complexities in the negative cases. It seems that we have to write some amount of code like yours. However, it'd be worth trying to avoid custom code and re-use CheckedNumeric and/or its family as much as possible. E.g. when positive, we may be able to leverage CheckedNumeric.
Austin Sullivan (Gerrit)
Austin Sullivan voted and added 4 comments![]( "Open in Gerrit")
Votes added by Austin Sullivan
| Code-Review | +1 |
4 comments
WebNN changes LGTM
#include "third_party/fp16/src/include/fp16.h"nit: is this needed anymore?
case webnn::OperandDataType::kFloat32:
// Implicit double -> float conversion.
return webnn::MLNumber::FromFloat32(bigint.ToDoubleLossy());
case webnn::OperandDataType::kFloat16:
// double (-> float, implicitly) -> fp16 (as uint16_t).
return webnn::MLNumber::FromFloat16(
fp16_ieee_from_fp32_value(bigint.ToDoubleLossy()));
case webnn::OperandDataType::kInt32: {
if (auto int32 = bigint.ToInt32()) {
return webnn::MLNumber::FromInt32(*int32);
} else {
return webnn::MLNumber::FromInt32(
bigint.IsNegative() ? std::numeric_limits<int32_t>::min()
: std::numeric_limits<int32_t>::max());
}
}
case webnn::OperandDataType::kUint32: {
if (auto uint32 = bigint.ToUInt32()) {
return webnn::MLNumber::FromUint32(*uint32);
} else {
return webnn::MLNumber::FromUint32(
bigint.IsNegative() ? std::numeric_limits<uint32_t>::min()
: std::numeric_limits<uint32_t>::max());
}
}Acknowledged
Related details
ningxin hu (Gerrit)
ningxin hu added 5 comments![]( "Open in Gerrit")
auto data_type = output_tensor_desc.GetDataType();
clamp_operator_desc.Min = ToScalarUnion(clamp->min_value, data_type);
clamp_operator_desc.Max = ToScalarUnion(clamp->max_value, data_type);
*/Remove the unused code.
static MLNumber FromFloat16(uint16_t number);
static MLNumber FromFloat32(float number);
static MLNumber FromFloat64(double number);
static MLNumber FromInt8(int8_t number);
static MLNumber FromUint8(uint8_t number);
static MLNumber FromInt32(int32_t number);
static MLNumber FromUint32(uint32_t number);
static MLNumber FromInt64(int64_t number);
static MLNumber FromUint64(uint64_t number);Only `FromFloat64`, `FromInt64` and `FromUint64` are useful now, you may want to delete the others.
bool ValidateClampAttributes(const mojom::Clamp& clamp,
webnn::OperandDataType data_type) {
return !clamp.min_value.IsGreaterThan(clamp.max_value, data_type);
}This helper seems to be trivial now. You may want to inline it.
#include "services/webnn/public/cpp/ml_number.h"
#include "services/webnn/public/cpp/operand_descriptor.h"These two headers are already included in ml_graph_type_converter.h
return static_cast<int64_t>(value);Yuki ShiinoOverall, it looks better to use CheckedNumeric<int64_t> if possible, avoiding the reinvention.
I think we can have something like the following code:
```
base::CheckedNumeric<int64_t> value(words_[0]);
if (!value.IsValid()) [[unlikely]] {
return std::nullopt;
}
return value.ValueOrDie();
```
I overlooked the complexities in the negative cases. It seems that we have to write some amount of code like yours. However, it'd be worth trying to avoid custom code and re-use CheckedNumeric and/or its family as much as possible. E.g. when positive, we may be able to leverage CheckedNumeric.
We may want to follow v8 `BigInt::AsInt64()` implementation: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/bigint.cc;l=1438;drc=a533a36dfe878045cd210166b8c3f1678fc987e7
Something like
```
uint64_t raw = words_[0];
// Simulate two's complement.
uint64_t raw = IsNegative() ? ((~raw) + 1u) : raw;
int64_t result = static_cast<int64_t>(raw);
if ((result < 0) != IsNegative()) return std::nullopt;
return result
```
@asu...@chromium.org, as you implemented the first version, please also take a look.
Related details
- Lisha Guo
Austin Sullivan (Gerrit)
Austin Sullivan added 2 comments![]( "Open in Gerrit")
static_cast<uint64_t>(std::abs(std::numeric_limits<int64_t>::min()))) {std::numeric_limits::min() returns a negative value, and in two's complement representation, the absolute value of that negative value cannot be represented in the same type. E.g. in case of signed char, min = -128 and max = 127, and abs(-128) cannot be represented in signed char.
I'm afraid that this code doesn't work as expected. It's nice to have a unittest to prove the behaviors in corner cases (with UB sanitizer).
https://en.cppreference.com/w/cpp/numeric/math/abs
> The behavior is undefined if the result cannot be represented by the return type.and
In 2's complement systems, the absolute value of the most-negative value is out of range, e.g. for 32-bit 2's complement type int, INT_MIN is -2147483648, but the would-be result 2147483648 is greater than INT_MAX, which is 2147483647.
It's nice to have a unittest to prove the behaviors in corner cases (with UB sanitizer).
+1 and apologies for my earlier comment on https://chromium-review.googlesource.com/c/chromium/src/+/6565815/comment/ee5c20ae_83fc719c/ - I forgot about the complexities involved with handling `IsNegative()`
@yukis...@chromium.org could you take a look at that thread and provide guidance on how to construct unit tests for this class, given the v8 complexities?
return static_cast<int64_t>(value);Yuki ShiinoOverall, it looks better to use CheckedNumeric<int64_t> if possible, avoiding the reinvention.
I think we can have something like the following code:
```
base::CheckedNumeric<int64_t> value(words_[0]);
if (!value.IsValid()) [[unlikely]] {
return std::nullopt;
}
return value.ValueOrDie();
```
ningxin huI overlooked the complexities in the negative cases. It seems that we have to write some amount of code like yours. However, it'd be worth trying to avoid custom code and re-use CheckedNumeric and/or its family as much as possible. E.g. when positive, we may be able to leverage CheckedNumeric.
We may want to follow v8 `BigInt::AsInt64()` implementation: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/bigint.cc;l=1438;drc=a533a36dfe878045cd210166b8c3f1678fc987e7
Something like
```
uint64_t raw = words_[0];
// Simulate two's complement.
uint64_t raw = IsNegative() ? ((~raw) + 1u) : raw;
int64_t result = static_cast<int64_t>(raw);
if ((result < 0) != IsNegative()) return std::nullopt;
return result
```@asu...@chromium.org, as you implemented the first version, please also take a look.
it'd be worth trying to avoid custom code and re-use CheckedNumeric and/or its family as much as possible
+1
Related details
- Lisha Guo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lisha Guo (Gerrit)
Lisha Guo added 7 comments![]( "Open in Gerrit")
auto data_type = output_tensor_desc.GetDataType();
clamp_operator_desc.Min = ToScalarUnion(clamp->min_value, data_type);
clamp_operator_desc.Max = ToScalarUnion(clamp->max_value, data_type);
*/Lisha GuoRemove the unused code.
Done
static MLNumber FromFloat16(uint16_t number);
static MLNumber FromFloat32(float number);
static MLNumber FromFloat64(double number);
static MLNumber FromInt8(int8_t number);
static MLNumber FromUint8(uint8_t number);
static MLNumber FromInt32(int32_t number);
static MLNumber FromUint32(uint32_t number);
static MLNumber FromInt64(int64_t number);
static MLNumber FromUint64(uint64_t number);Only `FromFloat64`, `FromInt64` and `FromUint64` are useful now, you may want to delete the others.
Done
bool ValidateClampAttributes(const mojom::Clamp& clamp,
webnn::OperandDataType data_type) {
return !clamp.min_value.IsGreaterThan(clamp.max_value, data_type);
}This helper seems to be trivial now. You may want to inline it.
Done
#include "services/webnn/public/cpp/ml_number.h"
#include "services/webnn/public/cpp/operand_descriptor.h"These two headers are already included in ml_graph_type_converter.h
Done
nit: is this needed anymore?
Done
static_cast<uint64_t>(std::abs(std::numeric_limits<int64_t>::min()))) {Austin Sullivanstd::numeric_limits::min() returns a negative value, and in two's complement representation, the absolute value of that negative value cannot be represented in the same type. E.g. in case of signed char, min = -128 and max = 127, and abs(-128) cannot be represented in signed char.
I'm afraid that this code doesn't work as expected. It's nice to have a unittest to prove the behaviors in corner cases (with UB sanitizer).
https://en.cppreference.com/w/cpp/numeric/math/abs
> The behavior is undefined if the result cannot be represented by the return type.and
In 2's complement systems, the absolute value of the most-negative value is out of range, e.g. for 32-bit 2's complement type int, INT_MIN is -2147483648, but the would-be result 2147483648 is greater than INT_MAX, which is 2147483647.
It's nice to have a unittest to prove the behaviors in corner cases (with UB sanitizer).+1 and apologies for my earlier comment on https://chromium-review.googlesource.com/c/chromium/src/+/6565815/comment/ee5c20ae_83fc719c/ - I forgot about the complexities involved with handling `IsNegative()`
@yukis...@chromium.org could you take a look at that thread and provide guidance on how to construct unit tests for this class, given the v8 complexities?
In 2's complement systems, the absolute value of the most-negative value is out of range, e.g. for 32-bit 2's complement type int, INT_MIN is -2147483648, but the would-be result 2147483648 is greater than INT_MAX, which is 2147483647.
@yukis...@chromium.org Maybe `base::SafeUnsignedAbs()` https://source.chromium.org/chromium/chromium/src/+/main:base/numerics/safe_conversions_impl.h;l=64 could solve this overflow issue?
Or we use the solution proposed by @ningx...@intel.com which following v8 BigInt::AsInt64() implementation?
was this change intentional?
No, it was deleted by accident. Added newlinear at end of the file back.
Related details
- Austin Sullivan
- Yuki Shiino
- ningxin hu
Lisha Guo (Gerrit)
Lisha Guo added 2 comments![]( "Open in Gerrit")
super nit: Better to remove this empty line as it looks inconsistent with other code blocks.
Done
Lisha Guowas this change intentional?
No, it was deleted by accident. Added newlinear at end of the file back.
Done
Yuki Shiino (Gerrit)
Yuki Shiino added 1 comment![]( "Open in Gerrit")
static_cast<uint64_t>(std::abs(std::numeric_limits<int64_t>::min()))) {Austin Sullivanstd::numeric_limits::min() returns a negative value, and in two's complement representation, the absolute value of that negative value cannot be represented in the same type. E.g. in case of signed char, min = -128 and max = 127, and abs(-128) cannot be represented in signed char.
I'm afraid that this code doesn't work as expected. It's nice to have a unittest to prove the behaviors in corner cases (with UB sanitizer).
https://en.cppreference.com/w/cpp/numeric/math/abs
> The behavior is undefined if the result cannot be represented by the return type.and
In 2's complement systems, the absolute value of the most-negative value is out of range, e.g. for 32-bit 2's complement type int, INT_MIN is -2147483648, but the would-be result 2147483648 is greater than INT_MAX, which is 2147483647.
Lisha GuoIt's nice to have a unittest to prove the behaviors in corner cases (with UB sanitizer).+1 and apologies for my earlier comment on https://chromium-review.googlesource.com/c/chromium/src/+/6565815/comment/ee5c20ae_83fc719c/ - I forgot about the complexities involved with handling `IsNegative()`
@yukis...@chromium.org could you take a look at that thread and provide guidance on how to construct unit tests for this class, given the v8 complexities?
In 2's complement systems, the absolute value of the most-negative value is out of range, e.g. for 32-bit 2's complement type int, INT_MIN is -2147483648, but the would-be result 2147483648 is greater than INT_MAX, which is 2147483647.
@yukis...@chromium.org Maybe `base::SafeUnsignedAbs()` https://source.chromium.org/chromium/chromium/src/+/main:base/numerics/safe_conversions_impl.h;l=64 could solve this overflow issue?
Or we use the solution proposed by @ningx...@intel.com which following v8 BigInt::AsInt64() implementation?
Given that we've already had a good implementation at `v8::internal::BigInt::AsInt64`, I don't think we need another approach (like using base::CheckedNumeric and/or base::SafeUnsignedAbs). I think it's best to copy the V8 impl into Blink unless we come up with a better one. \[[Cast from unsigned to signed](https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/bigint.cc;drc=a533a36dfe878045cd210166b8c3f1678fc987e7;l=1440) is an implementation-defined behavior though, I think it's acceptable as we never support 1's complement representation. It's not an undefined behavior.\]
About unittests, please add unittests in //third_party/blink/renderer/bindings/core/v8/, where you can use `V8TestingScope`. We already had such cases; the implementation lives in .../platform/bindings/ and its tests live in .../bindings/core/v8/, due to the exact same reason. It's fine.
I suppose that V8 may have good test cases already. Probably you can copy not only the implementation but also the test cases, too.
Related details
- Austin Sullivan
- Lisha Guo
- ningxin hu
ningxin hu (Gerrit)
ningxin hu added 1 comment![]( "Open in Gerrit")
Lisha has another plan and @wei4...@intel.com will pick up this work. Thanks both Lisha and Wei! Wei's first CL is ready for review: https://chromium-review.googlesource.com/c/chromium/src/+/6713775
Related details
- Austin Sullivan
- Lisha Guo
Dwayne Robinson (Gerrit)
Dwayne Robinson added 1 comment![]( "Open in Gerrit")
Abandon since https://chromium-review.googlesource.com/c/chromium/src/+/6713775 went in?
Related details
- Austin Sullivan
- Lisha Guo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dwayne Robinson (Gerrit)
Dwayne Robinson added 1 comment![]( "Open in Gerrit")
Abandon since https://chromium-review.googlesource.com/c/chromium/src/+/6713775 went in?
ningxin hu (Gerrit)
ningxin hu added 1 comment![]( "Open in Gerrit")
Dwayne RobinsonAbandon since https://chromium-review.googlesource.com/c/chromium/src/+/6713775 went in?
Followed by https://chromium-review.googlesource.com/c/chromium/src/+/6720936/2.
- Austin Sullivan
- Dwayne Robinson
- Lisha Guo
ningxin hu (Gerrit)
ningxin hu abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |