Added all possible elementwise binary ops for #316. by chudur-budur · Pull Request #344 · intel/mlir-extensions

chudur-budur · 2022-10-03T08:53:48Z

Please review these guidelines to help with the review process:

Have you provided a meaningful PR description?
- A draft PR for [ptensor] add missing elementwise binary operations #316. Added all possible element-wise binary ops.
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

fschlimb · 2022-10-04T09:38:55Z

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().


Why would we need buildTrivial if we use TOSA?

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

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 · 2022-10-04T09:40:14Z

lib/Conversion/PTensorToLinalg/PTensorToLinalg.cpp

+// Many TOSA functions take two arguments and do the
+// same operation on both arguments.


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

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

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

fschlimb · 2022-10-04T09:45:40Z

include/imex/Dialect/PTensor/IR/PTensorOps.h

  NOT_EQUAL,
  OR,
  POWER,
+  RSHIFT,


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

@fschlimb Okay, we can just duplicate them.

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

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?

I prefer BITWISE_*_SHIFT.

fschlimb · 2022-10-04T09:50:03Z

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 · 2022-10-05T05:13:24Z

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 · 2022-10-05T07:16:27Z

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

docs/rfcs/20220804-ptensor/README.md

chudur-budur · 2022-10-07T02:17:36Z

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 · 2022-10-07T08:24:38Z

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 · 2022-10-07T17:06:56Z

@fschlimb I think it's ready for merge.

fschlimb · 2022-10-17T13:33:21Z

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).

Added all possible elementwise binary ops for intel#316.

b27ca1f

chudur-budur requested review from diptorupd and fschlimb October 3, 2022 08:53

chudur-budur marked this pull request as ready for review October 4, 2022 05:58

fschlimb reviewed Oct 4, 2022

View reviewed changes

Addressing all fixes for intel#344

0d1a0c0

silee2 changed the base branch from refactor to main December 14, 2022 18:03

silee2 force-pushed the main branch from 1765695 to 70261a5 Compare March 20, 2023 22:47

		// Many TOSA functions take two arguments and do the
		// same operation on both arguments.

Conversation

chudur-budur commented Oct 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chudur-budur Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fschlimb Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chudur-budur Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fschlimb commented Oct 4, 2022

Uh oh!

chudur-budur commented Oct 5, 2022

Uh oh!

fschlimb commented Oct 5, 2022

Uh oh!

chudur-budur commented Oct 7, 2022

Uh oh!

fschlimb commented Oct 7, 2022

Uh oh!

chudur-budur commented Oct 7, 2022

Uh oh!

fschlimb commented Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chudur-budur Oct 5, 2022 •

edited

Loading

fschlimb Oct 5, 2022 •

edited

Loading

chudur-budur Oct 7, 2022 •

edited

Loading

fschlimb commented Oct 17, 2022 •

edited

Loading