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

[Lang] Matrix/Vector refactor: support basic matrix ops #6077

Merged
merged 21 commits into from
Sep 24, 2022

Conversation

AD1024
Copy link
Contributor

@AD1024 AD1024 commented Sep 15, 2022

Related issue = #5478
Refactored & Combined PR: #5797 and #5861
A part of PR #5551

@netlify
Copy link

netlify bot commented Sep 15, 2022

Deploy Preview for docsite-preview ready!

Name Link
🔨 Latest commit 486e6b3
🔍 Latest deploy log https://app.netlify.com/sites/docsite-preview/deploys/632e88a1a9bf640008e1774e
😎 Deploy Preview https://deploy-preview-6077--docsite-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@strongoier strongoier left a comment

Choose a reason for hiding this comment

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

  1. Shall we add tests for the modifications?
  2. Why both UnaryOp and BinaryOp are modified in codegen, but only BinaryOp is modified in frontend_ir?
  3. Shall we have is_real_scalar(), is_real_tensor(), and a general is_real() so that we don't have to use is_real() || is_real_tensor() everywhere?

taichi/ir/frontend_ir.cpp Outdated Show resolved Hide resolved
taichi/ir/frontend_ir.cpp Show resolved Hide resolved
taichi/transforms/type_check.cpp Outdated Show resolved Hide resolved
taichi/transforms/type_check.cpp Show resolved Hide resolved
Co-authored-by: Yi Xu <[email protected]>
Copy link
Contributor

@jim19930609 jim19930609 left a comment

Choose a reason for hiding this comment

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

You can probably separate out the codegen part, and add some c++ tests to test the type promotion and broadcasting.

Some examples can be found in: tests/cpp/ir/ir_type_promotion_test.cpp and tests/cpp/ir/frontend_type_inference_test.cpp

taichi/ir/frontend_ir.cpp Show resolved Hide resolved
taichi/ir/frontend_ir.cpp Outdated Show resolved Hide resolved
taichi/ir/frontend_ir.cpp Outdated Show resolved Hide resolved
taichi/ir/frontend_ir.cpp Show resolved Hide resolved
@AD1024
Copy link
Contributor Author

AD1024 commented Sep 21, 2022

  1. Shall we add tests for the modifications?
  2. Why both UnaryOp and BinaryOp are modified in codegen, but only BinaryOp is modified in frontend_ir?
  3. Shall we have is_real_scalar(), is_real_tensor(), and a general is_real() so that we don't have to use is_real() || is_real_tensor() everywhere?
  1. Sure! will add some tests to cover c++ side changes.
  2. The reason is we are only covering elementwise operations now at codegen. LLVM has corresponding variant of the common binary ops (+ - * /, etc.) for vector types so we don't need to change to. For UnaryOps, the major change is on casting because we need to cast every individual elements in a VectorType if a tensor is being cast to another tensor type.
  3. In the first version of the implementation I put them together but this change definitely caused some issue in other parts of the codebase. I tend to leave them as separate implementations.

python/taichi/lang/expr.py Outdated Show resolved Hide resolved
taichi/codegen/llvm/codegen_llvm.cpp Outdated Show resolved Hide resolved
taichi/codegen/llvm/codegen_llvm.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@jim19930609 jim19930609 left a comment

Choose a reason for hiding this comment

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

In codegen_llvm.cpp, for those LLVM operations that supports both Primitive and TensorType operands, let's simplify the condition from sth like is_real_tensor(stmt->ret_type) || is_real(ret_type) to is_real(ret_type.get_element_type()), so it's agnostic of whether it's TensorType or not.

python/taichi/lang/expr.py Outdated Show resolved Hide resolved
taichi/ir/type_utils.h Outdated Show resolved Hide resolved
@AD1024 AD1024 merged commit e89e6b1 into taichi-dev:master Sep 24, 2022
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