Skip to content

[fix][builtin] polymorphic builtins shouldn't inherit attributes #30435

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

Merged
merged 1 commit into from
Mar 18, 2020

Conversation

zoecarver
Copy link
Contributor

This PR makes all polymorphic builtins have empty attributes. Some attributes (such as read none) apply to the "regular" version of the builtin and not the polymorphic version. These builtins only exist in raw sil so, the error wasn't caught before. However, #30429 exposed the issue because mandatory combine runs during Onone. This is an NFC until #30429 lands.

@zoecarver zoecarver requested a review from gottesmm March 16, 2020 19:17
@zoecarver zoecarver force-pushed the fix/builtin-is-dead branch from e882852 to e18147e Compare March 16, 2020 19:48
@zoecarver zoecarver changed the title [fix][builtin] polymorphic builtins don't inherit attributes [fix][builtin] polymorphic builtins shouldn't inherit attributes Mar 16, 2020
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

@@ -113,7 +113,7 @@ BUILTIN_CAST_OR_BITCAST_OPERATION(SExtOrBitCast, "sextOrBitCast", "n")
#ifndef BUILTIN_BINARY_OPERATION_ALL
#define BUILTIN_BINARY_OPERATION_ALL(Id, Name, Attrs, Overload) \
BUILTIN_BINARY_OPERATION_OVERLOADED_STATIC(Id, BUILTIN_BINARY_OPERATION_GENERIC_HELPER_STR(Name), Attrs, Overload) \
BUILTIN_BINARY_OPERATION_POLYMORPHIC(Generic##Id, BUILTIN_BINARY_OPERATION_GENERIC_HELPER_STR(generic_##Name), Attrs)
BUILTIN_BINARY_OPERATION_POLYMORPHIC(Generic##Id, BUILTIN_BINARY_OPERATION_GENERIC_HELPER_STR(generic_##Name), "")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather change BUILTIN_BINARY_OPERATION_POLYMORPHIC to not take that flag at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I'll do that.

@zoecarver zoecarver requested a review from gottesmm March 17, 2020 04:17
Copy link
Contributor

@gottesmm gottesmm left a comment

Choose a reason for hiding this comment

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

Please squash the commits. But beyond that, LGTM.

@zoecarver zoecarver force-pushed the fix/builtin-is-dead branch from 4126786 to 8f15946 Compare March 17, 2020 22:17
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test and merge.

1 similar comment
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test and merge.

@swift-ci swift-ci merged commit 270f734 into swiftlang:master Mar 18, 2020
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.

3 participants