Skip to content

[mlir][complex] Make CPU runner test platform agnostic #85607

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 3 commits into from
Mar 19, 2024

Conversation

Lewuathe
Copy link
Member

The correctness.mlir test for complex dialects has been excluding the AArch64 platform. We have found that this is mainly due to the uncommon treatment of the Apple Silicon platform, which does not recognize signed NaN. We did not this behavior in AArch64 platform in general. Therefore, it is reasonable to make the test platform agnostic instead of keeping this test from running in the AArch64 platform, considering that the signness check of NaN value is not so essential here.

I confirmed this test passed in my Apple Silicon environment here.

$ uname -a
Darwin Kais-Mac.local 23.3.0 Darwin Kernel Version 23.3.0: Wed Dec 20 21:30:44 PST 2023; root:xnu-10002.81.5~7/RELEASE_ARM64_T6000 arm64

See more detail: #58531

@Lewuathe Lewuathe requested a review from banach-space March 18, 2024 07:04
@llvmbot llvmbot added mlir mlir:complex MLIR complex dialect labels Mar 18, 2024
@llvmbot
Copy link
Member

llvmbot commented Mar 18, 2024

@llvm/pr-subscribers-mlir-complex

@llvm/pr-subscribers-mlir

Author: Kai Sasaki (Lewuathe)

Changes

The correctness.mlir test for complex dialects has been excluding the AArch64 platform. We have found that this is mainly due to the uncommon treatment of the Apple Silicon platform, which does not recognize signed NaN. We did not this behavior in AArch64 platform in general. Therefore, it is reasonable to make the test platform agnostic instead of keeping this test from running in the AArch64 platform, considering that the signness check of NaN value is not so essential here.

I confirmed this test passed in my Apple Silicon environment here.

$ uname -a
Darwin Kais-Mac.local 23.3.0 Darwin Kernel Version 23.3.0: Wed Dec 20 21:30:44 PST 2023; root:xnu-10002.81.5~7/RELEASE_ARM64_T6000 arm64

See more detail: #58531


Full diff: https://github.com/llvm/llvm-project/pull/85607.diff

1 Files Affected:

  • (modified) mlir/test/Integration/Dialect/Complex/CPU/correctness.mlir (+2-5)
diff --git a/mlir/test/Integration/Dialect/Complex/CPU/correctness.mlir b/mlir/test/Integration/Dialect/Complex/CPU/correctness.mlir
index a42ed6968d3700..4a334b73baa25e 100644
--- a/mlir/test/Integration/Dialect/Complex/CPU/correctness.mlir
+++ b/mlir/test/Integration/Dialect/Complex/CPU/correctness.mlir
@@ -9,9 +9,6 @@
 // RUN:  -shared-libs=%mlir_c_runner_utils |\
 // RUN: FileCheck %s
 
-// XFAIL: target=aarch64{{.*}}
-// See: https://github.com/llvm/llvm-project/issues/58531
-
 func.func @test_unary(%input: tensor<?xcomplex<f32>>,
                       %func: (complex<f32>) -> complex<f32>) {
   %c0 = arith.constant 0 : index
@@ -189,8 +186,8 @@ func.func @entry() {
     // CHECK-NEXT:  0
     // CHECK-NEXT:  0
     (0.0, 0.0), (-1.0, 0.0),
-    // CHECK-NEXT:  -nan
-    // CHECK-NEXT:  -nan
+    // CHECK-NEXT:  nan
+    // CHECK-NEXT:  nan
     (1.0, 1.0), (1.0, 1.0)
     // CHECK-NEXT:  0.273
     // CHECK-NEXT:  0.583

Copy link
Contributor

@banach-space banach-space left a comment

Choose a reason for hiding this comment

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

Thank you so much for seeing this through, LGTM!

@Lewuathe Lewuathe force-pushed the gh-58531-platform-agnostic-test branch from 0f111a1 to a966c0f Compare March 19, 2024 05:18
@Lewuathe Lewuathe merged commit ccf042e into llvm:main Mar 19, 2024
chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mlir:complex MLIR complex dialect mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants