Skip to content
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

Added new tests for BLAS-like and BLAS API #4457

Closed

Conversation

kseniyazaytseva
Copy link
Contributor

Added tests for BLAS-like and BLAS functions to utest/ to increase code coverage.

Many functions did’t have test coverage at all (using test, ctest, utest and blas-tester). For example, with new tests code coverage for c910 optimized kernels reaches 95+% lines. Also, these tests helped to find new issues fixed in PR1 , PR2 and PR3

@martin-frbg
Copy link
Collaborator

Thank you - unfortunately it looks like test_extensions/CMakelists.txt appears to have a bunch of ${DIR_EXT} mis-spelled with non-curly braces, breaking the CMAKE build. And zero rows or columns in matcopy are no longer considered errors after #4306 by @angsch, I suspect the corresponding tests can be removed, if you agree ?

(Tests for ROTMG will probably cause lots of errors in other architectures as #4130 is not fixed yet, and it looks like your test cases have uncovered some long-standing bugs in the implementations of SCSUM/DZSUM on arm64 and x86_64 - which is a bit disconcerting as I simply threw out the fabs calls from the corresponding SCASUM/DZASUM kernel to create them...)

@martin-frbg
Copy link
Collaborator

It occured to me that some of the tests are flawed as they rely on uninitialized data, e.g. the samin_step_zero. Maybe these are implicitly initialized to zero on RISC-V (?), but this is not the case on other platforms. Just something to bear in mind for the eventual merge (which I intend to happen after testing on all relevant platforms, so that it does not suddenly break the build for everybody).

@kseniyazaytseva
Copy link
Contributor Author

kseniyazaytseva commented Feb 2, 2024

I've fixed cmake build and removed tests for zero rows or columns in matcopy.

ROTMG has one implementation for the entire platform, as I see. But the issue #4130 says about DROTG for which there are no tests here because it's covered by test/ctest.

Should we just remove tests for step zero cases?

@martin-frbg
Copy link
Collaborator

Tests for step zero cases do not need to be removed, but they should have x[0] (and x[1] in the complex cases) initialized to zero before the BLAS call. Right now they are uninitialized (the for loop for initialization with some variation of 1000-i is not executed, there is only a spurious setting of x[8] to zero) before the result is compared to zero. This leads to random failures if there happens to be something already in memory at the address pointed to by x.

@kseniyazaytseva
Copy link
Contributor Author

kseniyazaytseva commented Feb 2, 2024

Oh, then I suppose it's a bug in riscv (and probably arm) kernels. Because they all return explicit zero in case of zero step. And doesn't use any data in vectors.

Perhaps it would be better to protect this case in interfaces? What do you think?

@martin-frbg
Copy link
Collaborator

Could be a useful optimization - but conceptually I'm unsure what the correct behaviour is. When N is at least 1 and the increment is zero, wouldn't the return be determined by the value of the first array element ?

@kseniyazaytseva
Copy link
Contributor Author

Yes, I agree, that it neeed be determined by the value of the first array element. And I should rewrite these tests as you said. But in this case they may well cause failures on riscv(at least) if I set x[0] to be non-zero. Shouldn't the bugs be fixed first?

@martin-frbg
Copy link
Collaborator

Ah, I misunderstood. There will probably be bugs on several platforms - I think it would be best to merge the RISC-V branch into develop first, then submit the corrected tests as a PR against the develop branch, fix any kernels that show failures in CI, then finally merge the new tests into develop. This would probably cause the least disruption overall ?

@kseniyazaytseva
Copy link
Contributor Author

Yeah, we can do this. I've fixed tests for zero steps, rotg and some info returns. I also have small ones fixes for generic code (for z/c rotg, zero steps, geadd info returns and gemmt lost a line after merge). Will it be okay if we redirect this PR to develop branch and I commit my fixes here?

@martin-frbg
Copy link
Collaborator

yes please make this a PR against the develop branch, if possible

@martin-frbg
Copy link
Collaborator

closing as superseded by the merged #4499

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.

3 participants