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

Operator set wave 3 #805

Merged
merged 49 commits into from
Mar 11, 2025
Merged

Operator set wave 3 #805

merged 49 commits into from
Mar 11, 2025

Conversation

fdwr
Copy link
Collaborator

@fdwr fdwr commented Jan 16, 2025

Adds the following operators, per #375 (comment) Support for transformers:

Todos

Remaining Either done now, or deferred to separate CR/issue:

API

partial interface MLGraphBuilder
{
    ...
    MLOperand cumulativeSum(MLOperand input, unsigned long axis, optional MLCumulativeSumOptions options = {});
    MLOperand sign(MLOperand input, optional MLOperatorOptions options = {});
    MLOperand tile(MLOperand input, sequence<unsigned long> repetitions, optional MLOperatorOptions options = {});

    // Extends the family beyond the existing gather.
    MLOperand gatherElements(MLOperand input, MLOperand indices, optional MLGatherOptions options = {});
    MLOperand scatterElements(MLOperand input, MLOperand indices, MLOperand updates, optional MLScatterOptions options = {});
    MLOperand gatherND(MLOperand input, MLOperand indices, optional MLOperatorOptions options = {});
    MLOperand scatterND(MLOperand input, MLOperand indices, MLOperand updates, optional MLOperatorOptions options = {});

    MLOperand dequantizeLinear(MLOperand input, MLOperand scale, MLOperand zeroPoint, optional MLOperatorOptions options = {});
    MLOperand quantizeLinear(MLOperand input, MLOperand scale, MLOperand zeroPoint, optional MLOperatorOptions options = {});

    MLOperand logicalAnd(MLOperand a, MLOperand b, optional MLOperatorOptions options = {});
    MLOperand logicalOr(MLOperand a, MLOperand b, optional MLOperatorOptions options = {});
    MLOperand logicalXor(MLOperand a, MLOperand b, optional MLOperatorOptions options = {});
    MLOperand notEqual(MLOperand a, MLOperand b, optional MLOperatorOptions options = {});

    MLOperand reverse(MLOperand input, optional MLReverseOptions options = {});
    MLOperand slice(
        MLOperand input,
        sequence<[EnforceRange] unsigned long> starts,
        sequence<[EnforceRange] unsigned long> sizes,
        optional MLSliceOptions options = {} // Now includes steps
    );
    ...
}
dictionary MLCumulativeSumOptions : MLOperatorOptions
{
    boolean exclusive = false; // Post-sum addition rather than inclusive pre-sum. https://en.wikipedia.org/wiki/Prefix_sum
    boolean reversed = false; // Reverse the summation direction
}

// Already exists for `gather`. Reuse for `gatherElements` too.
dictionary MLGatherOptions : MLOperatorOptions
{
    unsigned long axis = 0;
};

dictionary MLScatterOptions : MLOperatorOptions
{
    unsigned long axis = 0;
};

dictionary MLReverseOptions : MLOperatorOptions
{
    sequence<[EnforceRange] unsigned long> axes;
};

dictionary MLSliceOptions : MLOperatorOptions
{
    sequence<[EnforceRange] unsigned long> strides;
};

Preview | Diff

@inexorabletash
Copy link
Member

Another "TODO" - the new ops need "constraints" tables

@fdwr fdwr changed the title Opset wave3 Operator set wave 3 Jan 16, 2025
@inexorabletash
Copy link
Member

Note from discussion w/ @a-sully - CoreML has restrictions on the dequantize op that we'll need to think about.

  • scale and bias must be constant
  • the input must be int8 or uint8 (note: CoreML has a different operator for (u)int4, but it requires everything - including the input - to be constant)
  • scale and bias must be either scalars or 1D
  • scale and bias must be the same rank (so, both scalars or both 1D)
  • scale must be the same data type as the output (note: the existing WPTs appear to assert the scale is always float32 (and the description of that test appears to have a typo))
  • scale must be positive

Re-emphasizing that dequantizing (u)int4 in CoreML is extremely limited (input must be const). @mwyrzykowski - any thoughts about how we can handle the proposed ops efficiently?

Copy link
Member

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

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

Initial pass.

@fdwr
Copy link
Collaborator Author

fdwr commented Jan 17, 2025

Another "TODO" - the new ops need "constraints" tables

Added data type tables.

Initial pass.

Thanks - will address more tomorrow after the weekend.

@inexorabletash
Copy link
Member

inexorabletash commented Jan 17, 2025

Add "Resolves #779" to the summary, so that issue will get linked to this PR and auto-closed when this merges?

... and "Resolves #773"
... and "Resolves #772"
... and "Resolves #467"
... and "Resolves #93" (I think?)

Maybe #767 too

Copy link
Collaborator Author

@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.

Addressed most feedback (not output shape calculation and blockwise compatible definition).

index.bs Outdated
</details>

### scatterElements ### {#api-mlgraphbuilder-scatterelements}
Scatter values from the updates tensor along an axis according to the indices in place of the input tensor.
Copy link
Collaborator Author

@fdwr fdwr Feb 13, 2025

Choose a reason for hiding this comment

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

🤔 What about "atop" in-place of inplace? Or in-place of a copy of?

Copy link
Member

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

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

Another batch of feedback; I didn't make it all the way through the PR though.

…ements examples, fix flatten on edge conditions
Copy link
Member

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@anssiko anssiko left a comment

Choose a reason for hiding this comment

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

🌊 Huge thank you to @fdwr for this significant op set update and @inexorabletash @huningxin, other participants for contributions and comments, in total close to 150 over 200!

As discussed, we'll periodically seek TAG review for significant changes to the spec. This spec update complemented with a demonstration of implementation experience across multiple backends and OSes will be brought to the TAG's attention alongside other significant changes.

Copy link
Member

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

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

Sorry, noted a few more places where an arg reference should be linkified rather than just styled.

Copy link
Collaborator Author

@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.

That should address all of Ningxin and Joshua's last feedback. 🤞

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!

@fdwr
Copy link
Collaborator Author

fdwr commented Mar 11, 2025

@inexorabletash Want to take one more pass?
If good, will merge.

@inexorabletash
Copy link
Member

I've been following along, still LGTM. Merge away!

@fdwr fdwr merged commit 6e19654 into webmachinelearning:main Mar 11, 2025
2 checks passed
github-actions bot added a commit that referenced this pull request Mar 11, 2025
SHA: 6e19654
Reason: push, by fdwr

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants