-
Notifications
You must be signed in to change notification settings - Fork 53
Support rankRange for op output tensors in opSupportLimits #857
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
base: main
Are you sure you want to change the base?
Conversation
/cc @philloooo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Ningxin. 👍
FYI, I am prototyping it in Chromium, will inform whether this PR is good to merge. |
f474e81
to
573f901
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fdwr @reillyeon , adding new changes that are based on Chromium prototype: https://chromium-review.googlesource.com/c/chromium/src/+/6903617, please take another look. Thanks!
@@ -2237,8 +2237,8 @@ partial dictionary MLOpSupportLimits { | |||
</tr> | |||
<tr> | |||
<td>*output*</td> | |||
<td>{{MLArgMinMaxOptions/outputDataType}}</td> | |||
<td>{{input}}'s [=MLOperand/rank=] - 1 to {{input}}'s [=MLOperand/rank=]</td> | |||
<td>{{MLOperandDataType/"int32"}}, {{MLOperandDataType/"int64"}}</td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This should list all allowed data types. The original outputDatatype is more useful for output data type calculation in the algorithm steps.
@@ -3421,7 +3421,7 @@ partial dictionary MLOpSupportLimits { | |||
<tr> | |||
<td>*output*</td> | |||
<td>[=/same type as|same as=] {{a}}</td> | |||
<td>maximum of {{a}}'s [=MLOperand/rank=] and {{b}}'s [=MLOperand/rank=]</td> | |||
<td>[=/any rank|N=]</td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: Same here and the following, the allowed ranks, not the output rank calculation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔
1. Add allowed ranks for all lstm and gru output tensors. 2. Validate new shape's rank of expand and reshape operations. 3. Simplify allowed ranks for some operations based on implementation experience. 4. Other minor changes for consistency.
9e651d8
to
356febd
Compare
Move the output tensor rank range info from constraints table to Returns section for argMin/Max and reduction ops. The element-wise binary and logical ops, matmul and where ops already have such info in the Returns section. Fix issues of output rank description of prelu and scatterND.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks NH.
This CL prototypes WebNN spec change [1] that supports tensor rank range for graph input, constant, output and each operation's output. This CL adds new shape rank validation according to allowed output tensor rank of expand and reshape operations. The sequence output of gru and cell operations has different rank, this CL extends context properties to support them. For logical ops, the output rank ranges are set to each op's input rank ranges, while the output data types are set to unified logical output data type of context properties. [1]: webmachinelearning/webnn#857 Bug: 442209350 Change-Id: Ie5ca5c794cebb4586bcccaaa8d237be1dffea458
This PR also supports rankRange for graph input, constant and output in opSupportLimits.
Fix #835
Preview | Diff