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

[ir] MatrixField refactor 1/n: Make a GlobalVariableExpression solely represent a field #5980

Merged
merged 3 commits into from
Sep 6, 2022

Conversation

strongoier
Copy link
Contributor

Related issue = #5959

Previously, a GlobalVariableExpression serves two purposes:

  • representing a field;
  • being a wrapper of a (maybe intermediate) SNode so that no matter a FrontendForStmt is actually a struct for on a SNode or an external tensor, it can always be constructed by passing an Expr.

However, the second purpose doesn't make sense - a struct for on a SNode and a struct for on an external tensor are handled differently everywhere (lower_ast, ir_printer, ...), so it's natural to split the constructors as well.

This PR clearly and consistently distinguishes different types of FrontendForStmt:

  • a struct for on a SNode (snode != nullptr);
  • a struct for on an external tensor (external_tensor != nullptr);
  • a mesh for (mesh != nullptr);
  • a range for (other).

As a result, the second purpose is eliminated and a GlobalVariableExpression serves solely for a field.

@strongoier strongoier changed the title [ir] MatrixField refactor 1/n: Make a GlobalVariableExpression solely represents a field [ir] MatrixField refactor 1/n: Make a GlobalVariableExpression solely represent a field Sep 5, 2022
Copy link
Contributor

@ailzhang ailzhang left a comment

Choose a reason for hiding this comment

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

LGTM!
nit: external_tensor isn't the best esp. when it comes to ndarrays. But this naming convention is kinda misleading but consistent in the current system lol. Shall we consider renaming them all together in a separate PR in the future?

@strongoier
Copy link
Contributor Author

LGTM! nit: external_tensor isn't the best esp. when it comes to ndarrays. But this naming convention is kinda misleading but consistent in the current system lol. Shall we consider renaming them all together in a separate PR in the future?

Sounds good. There are other misleading names like GlobalPtr, and we might need to systematically rename them all.

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