Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should Gather's indices data type be integers and support negative value? #484

Closed
huningxin opened this issue Dec 1, 2023 · 0 comments · Fixed by #642
Closed

Should Gather's indices data type be integers and support negative value? #484

huningxin opened this issue Dec 1, 2023 · 0 comments · Fixed by #642

Comments

@huningxin
Copy link
Contributor

In proposal Add support for operations needed for well-known transformers, the Gather's indices data type is defined as "uint32":

indices: an MLOperand. The indices N-D tensor of the input values to gather. The values must be of type "uint32" in the range [0, N-1] where N is the size of the input dimension indexed by options.axis.

However, as discussed in Chromium CL-5017758 review, frameworks and native ML APIs usually use signed integer and some of them support negative index values:

Frameworks

  • ONNX's Gather: indices data type is int32 or int64, supports negative index values, in range [-N, N-1]
  • TensorFlow's Gather: indices data type is int32 or int64, only supports non-negative index values, in range [0, N-1]

Native ML APIs

  • DirectML's Gather: indices data type is int64, int32, uint64 and uint32, supports negative index values
  • CoreML's Gather: indices data type is integers, support negative index values
  • StableHLO's Gather: indices data type is integers, doesn't support negative index values

Because the indices is an operand, its values may only available at run-time. If WebNN only supports "uint32" and range [0, N-1], frameworks that support negative indices, like ONNX, should polyfill (resolve the negative values) the indices tensor before passing to WebNN. WebNN implementation should enforce the restriction by polyfill the indices received from framework for all backends no matter whether the backend supports negative indices or not.

The best case would be single polyfill (frameworks supporting [0, N) + any backends), such as TF + DML, TF + StableHLO. The worst case might be double polyfill (frameworks support [-N, N) + any backends), such as ONNX + DML, ONNX + StableHLO.

The proposal is to support signed integers for Gather's indices and allow the negative index values, in range [-N, N-1] where N is the size of input dimensions indexed by options.axis.

With that, frameworks should just pass the indices tensor to WebNN, no polyfill needed. WebNN implementation would only need to polyfill the indices for the backends that don't support negative indices.

The best case would be non polyfill at all (any frameworks + backends supporting [-N, N)), such as ONNX + DML, TF + CoreML. The worst case might be single polyfill (any frameworks + backends supporting [0, N-1)), such as ONNX + StableHLO, TF + StableHLO.

/cc @wchao1115 @fdwr @wacky6

inexorabletash added a commit to inexorabletash/webnn that referenced this issue Apr 10, 2024
@fdwr fdwr closed this as completed in #642 Apr 16, 2024
fdwr added a commit that referenced this issue Apr 16, 2024
* gather(): Address indices validation and other algorithm nits

* #486 points out that indices can't be validated at build-time,
    and clamping behavior with an implementation note is given
    instead.

* Fix a typo in the steps.

* Replace several map-like iterations over lists with list iteration.

Fixes #486

* Update index.bs

Co-authored-by: Ningxin Hu <ningxin.hu@intel.com>

* Add note about negative indices. fixes #484

* Update index.bs

Co-authored-by: Dwayne Robinson <dwayner@microsoft.com>

* Update index.bs

Co-authored-by: Dwayne Robinson <dwayner@microsoft.com>

* Fix grammar glitch

---------

Co-authored-by: Ningxin Hu <ningxin.hu@intel.com>
Co-authored-by: Dwayne Robinson <dwayner@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants