Skip to content

[mlir][arith] Canonicalization patterns for arith.select #67809

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

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

peterbell10
Copy link
Contributor

@peterbell10 peterbell10 commented Sep 29, 2023

This adds the following canonicalization patterns:

  • Inverting condition:
    • select(not(pred), a, b) => select(pred, b, a)
  • Merging consecutive selects with the same predicate
    • select(pred, select(pred, a, b), c) => select(pred, a, c)
    • select(pred, a, select(pred, b, c)) => select(pred, a, c)
  • Merging consecutive selects with a common value:
    • select(predA, select(predB, a, b), b) => select(and(predA, predB), a, b)
    • select(predA, select(predB, b, a), b) => select(and(predA, not(predB)), a, b)
    • select(predA, a, select(predB, a, b)) => select(or(predA, predB), a, b)
    • select(predA, a, select(predB, b, a)) => select(or(predA, not(predB)), a, b)

@peterbell10
Copy link
Contributor Author

peterbell10 commented Oct 5, 2023

@matthias-springer I see you've reviewed a few mlir PRs recently. Would you be able to help find a reviewer for this? I can see that @llvmbot has pinged teams on other PRs but not here and am concerned it might be missed.

@peterbell10 peterbell10 force-pushed the arith-select-patterns branch from 437b36c to 55ac281 Compare October 5, 2023 17:03
This adds the following canonicalization patterns:
- Inverting condition:
  - `select(not(pred), a, b) => select(pred, b, a)`
- Merging consecutive selects with the same predicate
  - `select(pred, select(pred, a, b), c) => select(pred, a, c)`
  - `select(pred, a, select(pred, b, c)) => select(pred, a, c)`
- Merging consecutive selects with a common value value:
  - `select(predA, select(predB, x, y), y) => select(and(predA, predB), x, y)`
  - `select(predA, select(predB, y, x), y) => select(and(predA, not(predB)), x, y)`
  - `select(predA, x, select(predB, x, y)) => select(or(predA, predB), x, y)`
  - `select(predA, x, select(predB, y, x)) => select(or(predA, not(predB)), x, y)`
@peterbell10 peterbell10 force-pushed the arith-select-patterns branch from 55ac281 to cc15bcf Compare October 5, 2023 20:13
@peterbell10
Copy link
Contributor Author

@matthias-springer PTAL

@matthias-springer
Copy link
Member

Thanks! Looks good to me! Do you have commit access or do you need help landing this?

@peterbell10
Copy link
Contributor Author

No I don't have commit rights, this is actually my first LLVM contribution. Any help would be appreciated, thanks!

def SelectAndNotCond :
Pat<(SelectOp $predA, (SelectOp $predB, $y, $x), $y),
(SelectOp (Arith_AndIOp $predA,
(Arith_XOrIOp $predB, (Arith_ConstantOp ConstantAttr<I1Attr, "1">))),
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, this is wrong because it could be vector of i1 types, e.g., vector<4xi1>.

This patch introduces failure in downstream project (e.g., IREE). I'm working on a fix now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is a failure example:

  func.func @foo(%arg0: vector<4xf32>, %arg1: vector<4xf32>) -> vector<4xf32> {
    %0 = arith.cmpf ult, %arg0, %arg1 : vector<4xf32>
    %1 = arith.select %0, %arg0, %arg1 : vector<4xi1>, vector<4xf32>
    %2 = arith.cmpf uno, %arg1, %arg1 : vector<4xf32>
    %3 = arith.select %2, %arg1, %1 : vector<4xi1>, vector<4xf32>
    return %3 : vector<4xf32>
  }

stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
hanhanW added a commit that referenced this pull request Oct 13, 2023
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
@hanhanW
Copy link
Contributor

hanhanW commented Oct 13, 2023

As mentioned above, it broke the downstream project. So I reverted the commit. I have a WIP fix and will send it out for review.

stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
stellaraccident added a commit to iree-org/llvm-project that referenced this pull request Oct 13, 2023
…lvm#67809)"

This reverts commit 6668d14.

Missing support for vector<> types. Being reverted upstream and then relanded.
hanhanW added a commit that referenced this pull request Oct 13, 2023
…67809)" (#68941)

This cherry-picks the changes in
llvm-project/5bf701a6687a46fd898621f5077959ff202d716b and extends the
pattern to handle vector types.

To reuse `getBoolAttribute` method, it moves the static method above the
include of generated file.
stellaraccident added a commit to iree-org/iree that referenced this pull request Oct 16, 2023
Carrying local reverts for:

* #15083 (fixes still being made)
* llvm/llvm-project#67816 while we sync to a
corresponding TF version
* llvm/llvm-project#67809, which has already
been reverted at HEAD and will drop on its own

Landing with local revert: #15207

---------

Co-authored-by: Kunwar Grover <[email protected]>
Co-authored-by: Julian Walker <[email protected]>
ramiro050 pushed a commit to ramiro050/iree that referenced this pull request Dec 19, 2023
Carrying local reverts for:

* iree-org#15083 (fixes still being made)
* llvm/llvm-project#67816 while we sync to a
corresponding TF version
* llvm/llvm-project#67809, which has already
been reverted at HEAD and will drop on its own

Landing with local revert: iree-org#15207

---------

Co-authored-by: Kunwar Grover <[email protected]>
Co-authored-by: Julian Walker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants