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

Remove MLActivations definitely not usable with recurrent ops #703

Merged

Conversation

a-sully
Copy link
Contributor

@a-sully a-sully commented Jun 6, 2024

Follow-up to #664, without going so far as #689

Prior to #664, an MLActivation may be used for either op fusion or with recurrent operators (lstm and gru). Now it's only the latter, and some operators which may be created as an MLActivation no longer make sense to be passed as activations:

  • clamp
  • gelu (no longer being removed after feedback from reviewers)
  • softmax

Note that this PR is not a replacement for #689. That issue still stands on its own. Until we reach consensus on that issue, we should remove the MLActivations which were clearly targeted at the op fusion use case.


Preview | Diff

@a-sully
Copy link
Contributor Author

a-sully commented Jun 6, 2024

@huningxin FYI. This should simplify https://crrev.com/c/5495877

This list of operators is based on this code in Chromium's DML implementation. If there are others you think should be added to this PR, please let me know. I started with a minimal list to make this PR as uncontroversial as possible :)

Copy link
Collaborator

@fdwr fdwr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

index.bs Outdated Show resolved Hide resolved
@huningxin
Copy link
Contributor

@a-sully

@huningxin FYI. This should simplify https://crrev.com/c/5495877

Ack!

This list of operators is based on this code in Chromium's DML implementation.

Unfortunately that implementation may be mis-leading. They are not supported because they are only available in higher DirectML feature level, rather than they are not useful. I opened a Chromium issue regarding to that.

@fdwr

Note {ELU, CELU, RELU, GELU} are supported by DML RNN's including RNN, LSTM, GRU.

I suppose the current criteria is to keep activations that could be used by lstm and gru. So, should gelu be kept?

I don't find DirectML RNN ops supported activations list in MSDN, for example DML_GRU_OPERATOR_DESC. Did I miss anything?

@a-sully
Copy link
Contributor Author

a-sully commented Jun 7, 2024

Added back gelu based on feedback!

I don't find DirectML RNN ops supported activations list in MSDN, for example DML_GRU_OPERATOR_DESC. Did I miss anything?

Yup., that's why I was using the Chromium implementation as the source of truth. It would be nice to have these constraints documented! :P

Please merge at your convenience

@fdwr
Copy link
Collaborator

fdwr commented Jun 7, 2024

It would be nice to have these constraints documented! :P

Indeed - I'll follow up with our doc writer @stevewhims.

Copy link
Contributor

@huningxin huningxin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@huningxin huningxin merged commit f6a9af1 into webmachinelearning:main Jun 8, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Jun 8, 2024
SHA: f6a9af1
Reason: push, by huningxin

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@a-sully a-sully deleted the remove-some-mlactivations branch June 8, 2024 01:25
@fdwr
Copy link
Collaborator

fdwr commented Jun 8, 2024

I don't find DirectML RNN ops supported activations list in MSDN, for example DML_GRU_OPERATOR_DESC. Did I miss anything?

@huningxin : Until the docs are updated:

✅:
DML_OPERATOR_ACTIVATION_IDENTITY
DML_OPERATOR_ACTIVATION_LINEAR
DML_OPERATOR_ACTIVATION_ELU
DML_OPERATOR_ACTIVATION_RELU
DML_OPERATOR_ACTIVATION_CELU
DML_OPERATOR_ACTIVATION_GELU
DML_OPERATOR_ACTIVATION_SCALED_ELU
DML_OPERATOR_ACTIVATION_LEAKY_RELU
DML_OPERATOR_ACTIVATION_THRESHOLDED_RELU
DML_OPERATOR_ACTIVATION_PARAMETERIZED_RELU
DML_OPERATOR_ACTIVATION_PARAMETRIC_SOFTPLUS
DML_OPERATOR_ACTIVATION_SIGMOID
DML_OPERATOR_ACTIVATION_HARD_SIGMOID
DML_OPERATOR_ACTIVATION_SOFTPLUS
DML_OPERATOR_ACTIVATION_SOFTSIGN
DML_OPERATOR_ACTIVATION_TANH
DML_OPERATOR_ACTIVATION_SCALED_TANH
DML_OPERATOR_ACTIVATION_SHRINK
DML_OPERATOR_ACTIVATION_SWISH
DML_OPERATOR_ACTIVATION_HARD_SWISH

❌:
DML_OPERATOR_ELEMENT_WISE_CLIP
DML_OPERATOR_ACTIVATION_SOFTMAX
DML_OPERATOR_ACTIVATION_SOFTMAX1
DML_OPERATOR_ACTIVATION_LOG_SOFTMAX
DML_OPERATOR_ACTIVATION_LOG_SOFTMAX1
DML_OPERATOR_ACTIVATION_HARDMAX
DML_OPERATOR_ACTIVATION_HARDMAX1

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 10, 2024
clamp() and softmax() may no longer be created as MLActivations

More details on the spec PR:
webmachinelearning/webnn#703

Bug: 341518634
Cq-Include-Trybots: luci.chromium.try:win11-blink-rel,mac14.arm64-blink-rel,mac14-blink-rel
Change-Id: I7dda713c8be5454690f21c66665d90be274513f7
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 10, 2024
clamp() and softmax() may no longer be created as MLActivations

More details on the spec PR:
webmachinelearning/webnn#703

Bug: 341518634
Cq-Include-Trybots: luci.chromium.try:win11-blink-rel,mac14.arm64-blink-rel,mac14-blink-rel
Change-Id: I7dda713c8be5454690f21c66665d90be274513f7
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 10, 2024
clamp() and softmax() may no longer be created as MLActivations

More details on the spec PR:
webmachinelearning/webnn#703

Bug: 341518634
Change-Id: I7dda713c8be5454690f21c66665d90be274513f7
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 11, 2024
clamp() and softmax() may no longer be created as MLActivations

More details on the spec PR:
webmachinelearning/webnn#703

Bug: 341518634
Change-Id: I7dda713c8be5454690f21c66665d90be274513f7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5598089
Commit-Queue: Austin Sullivan <[email protected]>
Reviewed-by: Alex Gough <[email protected]>
Reviewed-by: ningxin hu <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1313205}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 11, 2024
clamp() and softmax() may no longer be created as MLActivations

More details on the spec PR:
webmachinelearning/webnn#703

Bug: 341518634
Change-Id: I7dda713c8be5454690f21c66665d90be274513f7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5598089
Commit-Queue: Austin Sullivan <[email protected]>
Reviewed-by: Alex Gough <[email protected]>
Reviewed-by: ningxin hu <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1313205}
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Jun 14, 2024
…e with recurrent ops, a=testonly

Automatic update from web-platform-tests
webnn: Remove some activations not usable with recurrent ops

clamp() and softmax() may no longer be created as MLActivations

More details on the spec PR:
webmachinelearning/webnn#703

Bug: 341518634
Change-Id: I7dda713c8be5454690f21c66665d90be274513f7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5598089
Commit-Queue: Austin Sullivan <[email protected]>
Reviewed-by: Alex Gough <[email protected]>
Reviewed-by: ningxin hu <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1313205}

--

wpt-commits: a632273a65dd5e8ba5f76ae0d03e6cb4f3affeee
wpt-pr: 46678
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jun 14, 2024
…e with recurrent ops, a=testonly

Automatic update from web-platform-tests
webnn: Remove some activations not usable with recurrent ops

clamp() and softmax() may no longer be created as MLActivations

More details on the spec PR:
webmachinelearning/webnn#703

Bug: 341518634
Change-Id: I7dda713c8be5454690f21c66665d90be274513f7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5598089
Commit-Queue: Austin Sullivan <[email protected]>
Reviewed-by: Alex Gough <[email protected]>
Reviewed-by: ningxin hu <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1313205}

--

wpt-commits: a632273a65dd5e8ba5f76ae0d03e6cb4f3affeee
wpt-pr: 46678
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Jun 16, 2024
…e with recurrent ops, a=testonly

Automatic update from web-platform-tests
webnn: Remove some activations not usable with recurrent ops

clamp() and softmax() may no longer be created as MLActivations

More details on the spec PR:
webmachinelearning/webnn#703

Bug: 341518634
Change-Id: I7dda713c8be5454690f21c66665d90be274513f7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5598089
Commit-Queue: Austin Sullivan <asullychromium.org>
Reviewed-by: Alex Gough <ajgochromium.org>
Reviewed-by: ningxin hu <ningxin.huintel.com>
Cr-Commit-Position: refs/heads/main{#1313205}

--

wpt-commits: a632273a65dd5e8ba5f76ae0d03e6cb4f3affeee
wpt-pr: 46678

UltraBlame original commit: 23cb27f7add77c95c93a44c1676157548f31abac
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants