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

Initial shot at structural equal and bug fixes in cpp_generator.py #15

Merged
merged 4 commits into from
Jun 17, 2021

Conversation

electriclilies
Copy link
Collaborator

I added a method to do structural equality and changed the way relax SEqualReduce methods are generated and also some basic structural equality tests

The structural equality method only works on functions not relax modules right now, since relax modules are just dictionaries right now (are we going to change the relax modules to be an object? if not then I can just loop through that dictionary in the assert_structural_equal method).

Copy link
Collaborator

@tkonolige tkonolige left a comment

Choose a reason for hiding this comment

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

Looks mostly good.

@@ -15,6 +15,7 @@ class ObjectField:
field_name: str
field_type: Type
is_binding: bool = False
check_equality: bool = True
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense for this to be called is_metadata?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the case of Spans, that makes sense, but I also had to set this false for function names. if function names can also count as metadata I think it works

Copy link
Collaborator

Choose a reason for hiding this comment

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

arguably function names are meta data if you are only looking at structure

Copy link
Owner

Choose a reason for hiding this comment

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

I think we should keep it generic potentially like use_in_{equal/hash/etc} this is pretty much deriving for all of object gen so it shouldn't be specific to AST imo

@@ -51,11 +52,6 @@ def __init__(self, definition_scope, diag_ctx):
self.module = {}
super().__init__()

def span_to_span(self, span):
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this code is equivalent to tvm_span_from_synr, we need to be really careful about the string -> source name mapping.

@jroesch jroesch merged commit 2300abe into jroesch:relax Jun 17, 2021
jroesch pushed a commit that referenced this pull request Jun 17, 2021
* Get initial version of structural equality working

* Fix typo in objectgen

* Cpp generator bug

* Respond to comments
jroesch pushed a commit that referenced this pull request Jun 17, 2021
* Get initial version of structural equality working

* Fix typo in objectgen

* Cpp generator bug

* Respond to comments
jroesch pushed a commit that referenced this pull request Jun 17, 2021
* Get initial version of structural equality working

* Fix typo in objectgen

* Cpp generator bug

* Respond to comments
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