webmachinelearning / webnn

🧠 Web Neural Network API
https://www.w3.org/TR/webnn/
Other
341 stars 42 forks source link

Introduce "valid dimension", used as needed when calculating operand shapes #641

Closed inexorabletash closed 1 month ago

inexorabletash commented 2 months ago

Certain operands like reshape2d() calculate dimensions in their output shape with expressions like shape[i] * scales[i] which could be larger than can be represented as a dimension in MLOperandDescriptor.

Introduce a "valid dimension" term, use it when calculating output dimensions in the following ops, and throw if needed.

Fixes #610


Preview | Diff

inexorabletash commented 2 months ago

Up for discussion. A few more thoughts here:

A valid dimension is an integer greater than zero in the range of unsigned long. Implementations may impose a lower upper limit.

(dropping "greater than zero" depending on #391)

... and then change check dimensions to say:

If any element of descriptor.dimensions is not a valid dimension, then return false.

Thoughts?

inexorabletash commented 2 months ago

If we introduce a "valid dimension" definition, we probably want to use it in these other places that calculate a dimension:

... which are the obvious places where a dimension is assigned from a calculation. Probably more places.

huningxin commented 2 months ago

Probably more places.

inexorabletash commented 2 months ago

Ah yes, it's a sum of other dimensions, so could overflow.

  • gru and gruCell, their steps compare a dimension with 3 * hiddenSize that might be a invalid unsigned long.
  • lstm and lstmCell, their steps compare a dimension with 4 * hiddenSize that might be a invalid unsigned long.

I think those are cases where the intermediate value could overflow if the math is done in uint32 space, but the final values used for dimensions are always valid uint32, since they come directly from other MLOperands. So those are cases where #636 applies.

I'm gonna update this PR to introduce the "valid dimension" term (done in e737dcb9b4f4fef332c7877116b1738221e50d3e). Should I go ahead and expand it beyond resample2d() or do that in another PR?

huningxin commented 2 months ago

Thanks @inexorabletash !

Should I go ahead and expand it beyond resample2d() or do that in another PR?

Given "valid dimension" is defined, having all its usages in this PR sounds good to me. But I am also fine if doing it in another PR.

inexorabletash commented 2 months ago

Given "valid dimension" is defined, having all its usages in this PR sounds good to me. But I am also fine if doing it in another PR.

Done in 4f30583c2b18c6fcebb6f178b14ac4ddcc12b725 - I'll update the PR name / description

inexorabletash commented 2 months ago

@fdwr - can you please review and merge if it looks good to you? Thanks!

inexorabletash commented 1 month ago

Suggested wording change was great @fdwr - it felt awkward as I was writing it.