Skip to content

Add isfinite() tests - #1530

Open
danbrown-amd wants to merge 4 commits into
llvm:mainfrom
danbrown-amd:isfinite
Open

danbrown-amd wants to merge 4 commits into
llvm:mainfrom
danbrown-amd:isfinite

Conversation

@danbrown-amd

Copy link
Copy Markdown
Collaborator

Resolves #919

@danbrown-amd

Copy link
Copy Markdown
Collaborator Author

Same data as isinf()/isnan() tests; same code except for the intrinsic invoked. Each filled output slot should be 1 iff the corresponding slots in the other two tests are both 0.

@kmpeng kmpeng left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My comments suggest slight changes to the scalar/vector tests that make them not completely parallel with isinf and isnan, but I think that's fine since those tests are kind of old

Comment thread test/Feature/HLSLLib/isfinite_mat.test Outdated
...
#--- end

# Bug https://github.com/llvm/llvm-project/issues/140824

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Don't think this is the right link

Suggested change
# Bug https://github.com/llvm/llvm-project/issues/140824
# Unimplemented https://github.com/llvm/llvm-project/issues/99131

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.

The bug applies as well, so I've included both lines.

@kmpeng kmpeng Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That bug was actually mislabeled for isinf and isnan. This was the real bug: llvm/llvm-project#226308, which was fixed. I'm guessing this is the real bug for isfinite too. Can you confirm? If it is, remove the bug comment and leave just the unimplemented one.

Note: I think we're going to run into a different Clang Vulkan bug once the isfinite implementation goes in (same one as isinf/isnan), but let's just update that later when we go to remove the unimplemented xfails.

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.

Done, though my PR addressing #140824 made those tests pass (for DirectX) as well. That PR is still needed to make the original example work.

Comment thread test/Feature/HLSLLib/isfinite.16.test Outdated
Comment thread test/Feature/HLSLLib/isfinite.16.test Outdated
Comment thread test/Feature/HLSLLib/isfinite.16.test
Comment thread test/Feature/HLSLLib/isfinite.16.test Outdated

@kmpeng kmpeng left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Edit: You likely need to remove the bug comment from the matrix test, then LGTM

LGTM, will get you another reviewer and merge it in for you once it's approved

Side note: did you want to request commit access to LLVM so you can merge PRs in yourself? You should have enough contributions to request it

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add test for isfinite

2 participants