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.

Sign up for GitHub

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

Jump to bottom

Added all possible elementwise binary ops for #316. #344

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Added all possible elementwise binary ops for #316. #344

wants to merge 2 commits into from

Conversation

chudur-budur
Copy link

Please review these guidelines to help with the review process:

  • Have you provided a meaningful PR description?
  • Have you added a test, a reproducer, or a reference to an issue with a reproducer?
    • The same test script should work with this.
  • Have you tested your changes locally for CPU and GPU devices?
    • Yes
  • Have you made sure that new changes do not introduce compiler warnings?
    • There are some new warnings, but they are not mitigated yet.
  • If this PR is a work in progress, are you filing the PR as a draft?
    • Yes
  • Have you organized your commits logically and ensured each can be built by itself?
    • Yes

@diptorupd @fschlimb

@chudur-budur chudur-budur marked this pull request as ready for review October 4, 2022 05:58
fschlimb
fschlimb reviewed Oct 4, 2022
View reviewed changes
lib/Conversion/PTensorToLinalg/PTensorToLinalg.cpp
// Many TOSA functions take two arguments and do the
// same operation on both arguments.
// TODO:
// 1. Find a way to merge this function with trivialBuilder().
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would we need buildTrivial if we use TOSA?

Copy link
Author

@chudur-budur chudur-budur Oct 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@fschlimb That's my thought too, should we just move to TOSA completely?

Copy link
Contributor

@fschlimb fschlimb Oct 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

When I investigated I realized that TOSA does not have unsigned types. I could not figure out if that's a gap or a design decision. If we can assume that TOSA can support unsigned ints then the only reason for not using TOSA might be compile-time. But I tend to think that's probably acceptable at this point.

fschlimb
fschlimb reviewed Oct 4, 2022
View reviewed changes
lib/Conversion/PTensorToLinalg/PTensorToLinalg.cpp
Comment on lines +300 to +301
// Many TOSA functions take two arguments and do the
// same operation on both arguments.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the functionality we need? Why is TOSA any better than math and arith?

Copy link
Author

Choose a reason for hiding this comment

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

@fschlimb I found those logical ops only in TOSA, is there any other alternative?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so. An alternative is of course to write them manually.

fschlimb
fschlimb reviewed Oct 4, 2022
View reviewed changes
include/imex/Dialect/PTensor/IR/PTensorOps.h Outdated
@@ -53,6 +53,7 @@ enum EWBinOpId : int {
NOT_EQUAL,
OR,
POWER,
RSHIFT,
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess LSHIFT and RSHIFT are duplicates of BITWISE_LEFT_SHIFT and BITWISE_RIGHT_SHIFT.

Copy link
Author

Choose a reason for hiding this comment

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

@fschlimb Okay, we can just duplicate them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I actually thought we should not. Any reason for doing so?

Copy link
Author

@chudur-budur chudur-budur Oct 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Well, actually there was LSHIFT but no RSHIFT, so I added it. Which ones we should keep? LSHIFT/RSHIFT or BITWISE_LEFT_SHIFT/BITWISE_RIGHT_SHIFT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer BITWISE_*_SHIFT.

@fschlimb
Copy link
Contributor

fschlimb commented Oct 4, 2022

Please also note that the RFC makes the following distinction:

  • elementwise_binary_op{$ebop}(lhs, rhs) : (ptensor.ptensor, ptensor.ptensor) -> ptensor
    • $ebop = ['add', 'sub', 'bitwise_left_shift', ...]
  • elementwise_binary_test{$ebop}(lhs, rhs) : (ptensor.ptensor, ptensor.ptensor) -> ptensor
    • $ebop = ['greater', 'less', 'equal', 'logical_and', ...]

@chudur-budur
Copy link
Author

Please also note that the RFC makes the following distinction:

  • elementwise_binary_op{$ebop}(lhs, rhs) : (ptensor.ptensor, ptensor.ptensor) -> ptensor

    • $ebop = ['add', 'sub', 'bitwise_left_shift', ...]

  • elementwise_binary_test{$ebop}(lhs, rhs) : (ptensor.ptensor, ptensor.ptensor) -> ptensor

    • $ebop = ['greater', 'less', 'equal', 'logical_and', ...]

Hi, I couldn't find this RFC, where is it?

@fschlimb
Copy link
Contributor

fschlimb commented Oct 5, 2022

Hi, I couldn't find this RFC, where is it?

docs/rfcs/20220804-ptensor/README.md

@chudur-budur
Copy link
Author

Hi, I couldn't find this RFC, where is it?

docs/rfcs/20220804-ptensor/README.md

What is the difference between elementwise_binary_op and elementwise_binary_test? Not clear to me.

@fschlimb
Copy link
Contributor

fschlimb commented Oct 7, 2022

What is the difference between elementwise_binary_op and elementwise_binary_test? Not clear to me.

The tests return a boolean tensor always. The others return numbers. If we do not need excessive conditionals in the implementation it is probably ok to combine.

@chudur-budur
Copy link
Author

@fschlimb I think it's ready for merge.

@fschlimb
Copy link
Contributor

fschlimb commented Oct 17, 2022 •
edited
Loading

I guess we need tests. I suggest deferring most of this until we resolved #380.

@chudur-budur Could you please add one test similar to test/imex-runner/ptensor_arange.mlir using one of the TOSA-backed binbops? You will need to extend test/imex-runner/ptensor.pp with the respective TOSA-passes (or add and use extended copy of it).

@silee2 silee2 changed the base branch from refactor to main December 14, 2022 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fschlimb fschlimb fschlimb left review comments

@diptorupd diptorupd Awaiting requested review from diptorupd

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chudur-budur @fschlimb