Conversation
farzonl
added
the
test-all
|
Also some of the tests seem to fail because there isn't an |
Sorry will do a review of this in the morning. But yeah xfail for clang for now |
kmpeng
left a comment
kmpeng
left a comment
There was a problem hiding this comment.
You should take [Matrix] out of the title since this PR is covering more than just the matrix tests.
kmpeng
left a comment
kmpeng
left a comment
There was a problem hiding this comment.
The structure of the tests lgtm, but I have some problems with the values being used. I'm also not very familiar with matrices, so I'll defer to other people for more feedback on those (@farzonl ?)
FYI a lot of the failing checks on this PR have been fixed recently. Could you update the branch so it's easier to see the ones this PR is affecting?
test/Feature/HLSLLib/fma.test
Outdated
| Stride: 32 | ||
| Data: [ 0.25, -0.25, 10.4, -10.6, | ||
| 1.5, -2.5, 0.5, -0.5, | ||
| 0.0, 10.0, -10.0, 10.5 ] |
There was a problem hiding this comment.
I think you need to use more meaningful values in these tests. The purpose of fma is that it's more precise than just a*b+c - it only rounds once at the end of the operation while a*b+c rounds twice (once after multiply, once after add). Your current test values are mostly trivial (there's lots of x*1.0+0.0 identity operations) and all the values are exactly representable in fp64, so we're not testing any of the intermediate precision loss that matters with fma.
Having some simple tests is important, but I do think we need some more robust ones here
There was a problem hiding this comment.
Do i need to do this for matrix too or only here is fine?
There was a problem hiding this comment.
Should get other people's opinions on this who are more familiar with matrices. But from my understanding, matrix tests in general are more about verifying dimensions/shapes, so they don't have to be as comprehensive as the scalar/vector ones
|
Sorry was a little busy today, will get to this tomorrow |
…es as in constant folding
kmpeng
left a comment
kmpeng
left a comment
There was a problem hiding this comment.
LGTM, but again you should probably get more feedback on the matrix test
|
Could you maybe ping someone who is working with matrices then? I would be really grateful. |
6 participants
This PR adds double tests for
fmafunction. Only double is applicable to this function as per official apiCloses #916
Related to llvm/llvm-project/#185304
Assisted-by: GPT-5.4 (opencode)