Skip to content

Adds tests for isnan().#466

Open
danbrown-amd wants to merge 1 commit intollvm:mainfrom
danbrown-amd:isnan
Open

Adds tests for isnan().#466
danbrown-amd wants to merge 1 commit intollvm:mainfrom
danbrown-amd:isnan

Conversation

@danbrown-amd
Copy link
Copy Markdown
Collaborator

Test values taken from the isinf() tests; others may be relevant.

Comment thread test/Feature/HLSLLib/isnan.16.test Outdated
...
#--- end

# llvm/llvm-project#141089
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the issue you linked is closed llvm/llvm-project#141089 are you sure this should still xfail for clang-vulkan?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Clang-Vulkan XFAIL removed.

Comment thread test/Feature/HLSLLib/isnan.16.test Outdated
# llvm/llvm-project#141089
# XFAIL: Clang-Vulkan

# https://github.com/llvm/llvm-project/issues/145571
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We added the isnan emulation see https://github.com/llvm/llvm-project/pull/157505/files this should work for clang.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I don't have an nVidia GPU, so I suppose I can't test for DirectX-NV?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't run the nvidia gpu runner yet on the PR runs. and the only way to manually invoke it would be if you had a branch on this repository. There doesn't apear to be anyway to do that with a fork. That said I feel confident that if you are seeing the emulation passing on amd\intel for sm 6.8 and lower then it should pass for nvidia too since we won't be emitting the opcode that would blow things up.

Copy link
Copy Markdown
Contributor

@damyanp damyanp Oct 23, 2025

Choose a reason for hiding this comment

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

If you add a test-all label to the PR then it should run against all the machines in the offload tester lab.

https://github.com/llvm/offload-test-suite/blob/main/docs/CI.md

If this doesn't work for PRs from forks we should update these docs!

@damyanp damyanp added the test-all When applied to a PR this will opt-in to additional pre-merge test configurations.. label Oct 23, 2025
@farzonl
Copy link
Copy Markdown
Member

farzonl commented Dec 10, 2025

All the tests are failing could you update the branch?

@danbrown-amd
Copy link
Copy Markdown
Collaborator Author

danbrown-amd commented Dec 10, 2025

I'm just now getting back to this and still get the original error on my side.

@danbrown-amd
Copy link
Copy Markdown
Collaborator Author

@farzonl would you please rerun the tests so I can see whether I still need XFAIL for NV/QC?

Copy link
Copy Markdown
Member

@farzonl farzonl left a comment

Choose a reason for hiding this comment

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

test LGTM.

Will just want to confirm test passes across vendors before merging.
If you see issues feel free to file issues and add vendor specific xfails for now.

@danbrown-amd
Copy link
Copy Markdown
Collaborator Author

On the last test run, 8 tests passed, 9 failed with errors in previously existing tests, and 6 did not run to completion. I rebased and updated on my end, with the only change in the added tests being that I removed some obsolete comment lines.

@farzonl
Copy link
Copy Markdown
Member

farzonl commented Mar 13, 2026

@danbrown-amd is this ready to be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test-all When applied to a PR this will opt-in to additional pre-merge test configurations..

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants