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

[AutoBump] Merge with fixes of f18c3e4e (Oct 23) (18) (Needs ONNX Bump)(Needs Torch Bump) #455

Merged
merged 3 commits into from
Jan 27, 2025

Conversation

jorickert
Copy link

@jorickert jorickert commented Jan 15, 2025

matthias-springer and others added 3 commits October 23, 2024 07:29
…sult type (llvm#113031)

This commit simplifies the result type of materialization functions.

Previously: `std::optional<Value>`
Now: `Value`

The previous implementation allowed 3 possible return values:
- Non-null value: The materialization function produced a valid
materialization.
- `std::nullopt`: The materialization function failed, but another
materialization can be attempted.
- `Value()`: The materialization failed and so should the dialect
conversion. (Previously: Dialect conversion can roll back.)

This commit removes the last variant. It is not particularly useful
because the dialect conversion will fail anyway if all other
materialization functions produced `std::nullopt`.

Furthermore, in contrast to type conversions, at least one
materialization callback is expected to succeed. In case of a failing
type conversion, the current dialect conversion can roll back and try a
different pattern. This also used to be the case for materializations,
but that functionality was removed with llvm#107109: failed materializations
can no longer trigger a rollback. (They can just make the entire dialect
conversion fail without rollback.) With this in mind, it is even less
useful to have an additional error state for materialization functions.

This commit is in preparation of merging the 1:1 and 1:N type
converters. Target materializations will have to return multiple values
instead of a single one. With this commit, we can keep the API simple:
`SmallVector<Value>` instead of `std::optional<SmallVector<Value>>`.

Note for LLVM integration: All 1:1 materializations should return
`Value` instead of `std::optional<Value>`. Instead of `std::nullopt`
return `Value()`.
@jorickert jorickert changed the title [AutoBump] Merge with fixes of f18c3e4e (Oct 23) (18) [AutoBump] Merge with fixes of f18c3e4e (Oct 23) (18) (Needs ONNX Bump) Jan 15, 2025
@jorickert jorickert marked this pull request as draft January 15, 2025 07:06
@jorickert jorickert changed the title [AutoBump] Merge with fixes of f18c3e4e (Oct 23) (18) (Needs ONNX Bump) [AutoBump] Merge with fixes of f18c3e4e (Oct 23) (18) (Needs ONNX Bump)(Needs Torch Bump) Jan 20, 2025
Base automatically changed from bump_to_8a9921f5 to bump_to_519eef3b January 24, 2025 08:43
Base automatically changed from bump_to_519eef3b to bump_to_0a17bdfc January 24, 2025 08:44
@jorickert jorickert marked this pull request as ready for review January 27, 2025 07:26
@jorickert jorickert merged commit 41d0253 into bump_to_0a17bdfc Jan 27, 2025
6 checks passed
@jorickert jorickert deleted the bump_to_f18c3e4e branch January 27, 2025 07:26
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.

2 participants