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

[Type] Adopt the new type system in Stmt #1957

Merged
merged 18 commits into from
Oct 15, 2020

Conversation

yuanming-hu
Copy link
Member

@yuanming-hu yuanming-hu commented Oct 14, 2020

Related issue = #1905

Finally, we can start using the new type system. This PR serves as one major refactoring step that uses DataType instead of (Legacy)VectorType in the Stmt class, where type inference works on. The migration is unfortunately non-trivial, since types are used everywhere in the Taichi compiler code.

To simplify review and ensure correctness, the overall strategy for this large-scale refactoring is

  1. Use DataType for all the places where Type * is used. Since DataType is a class, we can abuse tricks such as operator overloading to smoothly refactor while preserving the old behavior and minimize mechanical code changes.
  2. Use @taichi-gardener to do mechanical replacements.

This PR belongs to type 1. Later I'll open a few PRs of type 2 after this is in.

Major changes:

  • The old LegacyVectorType class is now removed.
  • LegacyVectorType is now a function. It returns a DataType. We can later easily remove the LegacyVectorType function.
  • Added a temporary DataType::data_type member, which is a reference to the DataType itself. This is just to minimize the changes, since stmt->ret_type.data_type was a popular pattern in the old code. This member will be removed later.
  • Added RTTI utils such as is/as/cast to Type.
  • One core feature of the new type system is that pointers are explicitly supported. The old type system doesn't distinguish pointer/non-pointer types. Therefore you will see some ptr_removed/set_is_pointer function calls that patches the old backend code to ensure pointer/non-pointer types are no longer mixed together.

Many members of DataType, as well as the class DataType is self, are just auxiliary, and will be removed later.

@k-ye could you test if Metal still works well on your Mac? I tried to fix a few clear issues, but my 7-year-old MBP fails 50+ tests before & after this PR so I can't really confirm if my fixes are good enough. If anything goes wrong, most likely adding ptr_removed() somewhere can get the issue fixed.

[Click here for the format server]


Comment on lines -534 to +497
LegacyVectorType ret_type;
DataType ret_type;
Copy link
Member Author

Choose a reason for hiding this comment

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

The centric change. Every other change in this PR is around this line.

@codecov
Copy link

codecov bot commented Oct 14, 2020

Codecov Report

Merging #1957 into master will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1957   +/-   ##
=======================================
  Coverage   43.64%   43.64%           
=======================================
  Files          45       45           
  Lines        6225     6225           
  Branches     1106     1106           
=======================================
  Hits         2717     2717           
  Misses       3334     3334           
  Partials      174      174           
Impacted Files Coverage Δ
python/taichi/lang/util.py 32.93% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45f52a9...e3e859b. Read the comment docs.

@TH3CHARLie
Copy link
Collaborator

Great job! I haven't done a full review(will do it soon) but it makes the most sense to me that I should rebase my work on this after this is merged.

@yuanming-hu
Copy link
Member Author

Great job! I haven't done a full review(will do it soon) but it makes the most sense to me that I should rebase my work on this after this is merged.

Thank you! Please take your time. Sorry about the large change here!

Copy link
Contributor

@Hanke98 Hanke98 left a comment

Choose a reason for hiding this comment

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

Great! This is a hard work and most of the refactoring LGTM. The only thing that I am not so sure is where we use pointer type and where not and I have left my question here.
Thanks, I learnt a lot from the refactoring !

taichi/backends/opengl/codegen_opengl.cpp Show resolved Hide resolved
@Hanke98 Hanke98 self-requested a review October 15, 2020 05:14
Copy link
Contributor

@Hanke98 Hanke98 left a comment

Choose a reason for hiding this comment

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

Thanks a lot, I have no question now! : )

Copy link
Collaborator

@TH3CHARLie TH3CHARLie left a comment

Choose a reason for hiding this comment

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

LGTM! I especially appreciate the tricks that help to keep migration changes minimal. That's very insightful to me

I only felt a little bit of confusion about the name of ptr_removed at my first glance, not sure if there's a better name but since the entirety of DataType would be updated/removed soon, this can be safely ignored.

I think this PR can be merged once @k-ye confirms the compilation of metal backend.

@Hanke98
Copy link
Contributor

Hanke98 commented Oct 15, 2020

I only felt a little bit of confusion about the name of ptr_removed at my first glance, not sure if there's a better name but since the entirety of DataType would be updated/removed soon, this can be safely ignored.

Yes, you are right, I felt confused too at the beginning. This function and class might be a temporary solution when the refactoring is in progress. So just as you say, let's ignored it for now.

Copy link
Member

@k-ye k-ye left a comment

Choose a reason for hiding this comment

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

Thanks for confirming, LGTM!

Metal tests all passed 😄 (I did a rebase master, though. But it could be that your MBP is still suffering the memory order issue...)

I only felt a little bit of confusion about the name of ptr_removed at my first glance

I wonder if maybe_ptr_removed() is a better name, which implies that the data type could be a pointer type. (OTOH, it could sound like whether removal happens is indeterministic..) But overall I think this is pretty good.

@yuanming-hu
Copy link
Member Author

LGTM! I especially appreciate the tricks that help to keep migration changes minimal. That's very insightful to me

I did attempt a few other approaches and they all resulted in much larger changes :-) Hopefully that justifies the weird tricks I play here :-)

I only felt a little bit of confusion about the name of ptr_removed at my first glance, not sure if there's a better name but since the entirety of DataType would be updated/removed soon, this can be safely ignored.

I think this PR can be merged once @k-ye confirms the compilation of metal backend.

Yes, you are right, I felt confused too at the beginning. This function and class might be a temporary solution when the refactoring is in progress. So just as you say, let's ignored it for now.

I wonder if maybe_ptr_removed() is a better name, which implies that the data type could be a pointer type. (OTOH, it could sound like whether removal happens is indeterministic..) But overall I think this is pretty good.

Thanks for the feedback! I agree ptr_removed() is indeed confusing. Fortunately this is just a temporary solution and I'll definitely choose a different name later in the refactoring. After the pointer/non-pointer types are strictly enforced, we should also get rid of indeterministic behaviors of something like maybe_ptr_removed().

Metal tests all passed (I did a rebase master, though. But it could be that your MBP is still suffering the memory order issue...)

Thanks for testing! Yeah, it's probably memory order and perhaps SIMD. My MBP is too old and not worth supporting anymore.

@yuanming-hu yuanming-hu merged commit 6b3db04 into taichi-dev:master Oct 15, 2020
@yuanming-hu yuanming-hu mentioned this pull request Oct 17, 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.

4 participants