Add isfinite() tests - #1530
Add isfinite() tests#1530danbrown-amd wants to merge 4 commits into
Conversation
9aa04f9 to
9666cfa
Compare
|
Same data as |
9666cfa to
c3bdcbf
Compare
kmpeng
left a comment
There was a problem hiding this comment.
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
| ... | ||
| #--- end | ||
|
|
||
| # Bug https://github.com/llvm/llvm-project/issues/140824 |
There was a problem hiding this comment.
Don't think this is the right link
| # Bug https://github.com/llvm/llvm-project/issues/140824 | |
| # Unimplemented https://github.com/llvm/llvm-project/issues/99131 |
There was a problem hiding this comment.
The bug applies as well, so I've included both lines.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
9ab2696 to
69776b4
Compare
69776b4 to
ca65b81
Compare
Resolves #919