Skip to content

[mlir][arith] Improve Lowering of maxf/minf ops #65213

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
Sep 6, 2023

Conversation

unterumarmung
Copy link
Contributor

@unterumarmung unterumarmung commented Sep 2, 2023

This patch is part of a larger initiative aimed at fixing floating-point max and min operations in MLIR: https://discourse.llvm.org/t/rfc-fix-floating-point-max-and-min-operations-in-mlir/72671.

This patch addresses task 1.1 from the plan. It involves modifying the lowering process for arith.minf and arith.maxf operations. Specifically, the change replaces the usage of llvm.minnum and llvm.maxnum with llvm.minimum and llvm.maximum, respectively. This adjustment is necessary because the m**num intrinsics are not suitable for the mentioned MLIR operations due to semantic discrepancies in handling NaNs, positive and negative floating-point zeros.

@unterumarmung unterumarmung requested a review from a team as a code owner September 2, 2023 21:26
@unterumarmung
Copy link
Contributor Author

The patch has been moved from https://reviews.llvm.org/D158865

This patch is part of a larger initiative aimed at fixing floating-point `max` and `min` operations in MLIR: https://discourse.llvm.org/t/rfc-fix-floating-point-max-and-min-operations-in-mlir/72671.

This patch addresses task 1.1 from the plan. It involves modifying the lowering process for `arith.minf` and `arith.maxf` operations. Specifically, the change replaces the usage of `llvm.minnum` and `llvm.maxnum` with `llvm.minimum` and `llvm.maximum`, respectively. This adjustment is necessary because the `m**num` intrinsics are not suitable for the mentioned MLIR operations due to semantic discrepancies in handling NaNs, positive and negative floating-point zeros.

Differential Revision: https://reviews.llvm.org/D158865
@github-actions github-actions bot added the mlir label Sep 6, 2023
@unterumarmung unterumarmung merged commit d4fa088 into llvm:main Sep 6, 2023
unterumarmung added a commit to unterumarmung/llvm-project that referenced this pull request Sep 8, 2023
Following llvm#65213, the optimization of the `arith.maxf` operation has transitioned from using the `maxnum` LLVM intrinsic to `maximum`. This modification renders the statement in the previous commit obsolete and the associated workaround unnecessary. Consequently, this commit removes the workaround and rectifies related test cases.
avillega pushed a commit to avillega/llvm-project that referenced this pull request Sep 11, 2023
This patch is part of a larger initiative aimed at fixing floating-point
`max` and `min` operations in MLIR:
https://discourse.llvm.org/t/rfc-fix-floating-point-max-and-min-operations-in-mlir/72671.

This patch addresses task 1.1 from the plan. It involves modifying the
lowering process for `arith.minf` and `arith.maxf` operations.
Specifically, the change replaces the usage of `llvm.minnum` and
`llvm.maxnum` with `llvm.minimum` and `llvm.maximum`, respectively. This
adjustment is necessary because the `m**num` intrinsics are not suitable
for the mentioned MLIR operations due to semantic discrepancies in
handling NaNs, positive and negative floating-point zeros.
dcaballe pushed a commit to iree-org/llvm-project that referenced this pull request Sep 14, 2023
This patch is part of a larger initiative aimed at fixing floating-point
`max` and `min` operations in MLIR:
https://discourse.llvm.org/t/rfc-fix-floating-point-max-and-min-operations-in-mlir/72671.

This patch addresses task 1.1 from the plan. It involves modifying the
lowering process for `arith.minf` and `arith.maxf` operations.
Specifically, the change replaces the usage of `llvm.minnum` and
`llvm.maxnum` with `llvm.minimum` and `llvm.maximum`, respectively. This
adjustment is necessary because the `m**num` intrinsics are not suitable
for the mentioned MLIR operations due to semantic discrepancies in
handling NaNs, positive and negative floating-point zeros.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants