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

Fixed comparison between two Type_Bits #3377

Merged
merged 2 commits into from
Jun 2, 2022

Conversation

MichalKekely
Copy link
Contributor

Due to the changes where new Type_Bits are created the "==" comparison no longer works, equiv is used instead.

* Due to the changes where new Type_Bits are created
  the "==" comparison no longer works, equiv is used instead.
@@ -1837,7 +1837,8 @@ const IR::Node* TypeInference::postorder(IR::Operation_Relation* expression) {
expression->left = c.left;
expression->right = c.right;
} else {
if (!ltype->is<IR::Type_Bits>() || !rtype->is<IR::Type_Bits>() || !(ltype == rtype)) {
if (!ltype->is<IR::Type_Bits>() || !rtype->is<IR::Type_Bits>() || !(ltype->equiv(*rtype))) {
std::cerr << "Here" << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

this should not be here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, removed :-)

@mihaibudiu
Copy link
Contributor

If you can add a test that fails without this it's also useful.

@MichalKekely
Copy link
Contributor Author

If you can add a test that fails without this it's also useful.

Just working on it

@MichalKekely
Copy link
Contributor Author

MichalKekely commented Jun 2, 2022

If you can add a test that fails without this it's also useful.

Well, this seems to be quite specific issue with a non-open source midend. The issue is created by not properly clearing the TypeMap before TypeChecking after some pass did transformations that reintroduced the same Type_Bits, however with different id.

It seems to be fixable in the specific midend also by properly clearing the TypeMap, but I would still consider going forward with this change as well. Let's assume we have these two types:
[1502] Type_Bits size=16 isSigned=0
[6970] Type_Bits size=16 isSigned=0
It would make sense that these two should be comparable (since both of these Type_Bits objects clearly represent the same type). However == returns false for these (and therefore the original if statement is triggered), while equiv returns true (and therefore the changed if statement is not triggered). And it also does not make much sense when the actual error message looks like:
[--Werror=type-error] error: >=: not defined on bit<16> and bit<16>

@mihaibudiu
Copy link
Contributor

The original design for the typeMap called for using "canonical type representations" which could be compared for equality. There is a bunch of code in the compiler trying to do that. However, it's not clear that "canonical" is well-defined when dealing with generics, so equiv() or unification are the right way to check types for "equality". The spec does not really say when two types are equal either.

@mihaibudiu mihaibudiu merged commit 29fe3c5 into main Jun 2, 2022
@mihaibudiu mihaibudiu deleted the mkekely/type_bits_comparisons_fix branch June 2, 2022 14:13
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