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

Updates the documentation of xGEMV and xGBMV related to when M=0 and N=0 #843

Merged
merged 2 commits into from
Aug 22, 2023

Conversation

weslleyspereira
Copy link
Collaborator

@weslleyspereira weslleyspereira commented May 26, 2023

Closes #248.
Closes #788.

The documentation of GEMV (and GBMV) misses a boundary case, namely when M=0 or N=0. There are two possibilities for the solution of this issue:

  1. The behavior of GEMV (and GBMV) does not seem to match the behavior of GEMM, SYRK, SYR2K, HERK and HER2K regarding matrices with zero input sizes. In GEMM, for instance, C gets updated even if the internal dimension K is zero. In GEMV (and GBMV), if the internal dimension (M or N) is zero, Y does not get updated. The first solution is, therefore, to change the condition in GEMV (and GBMV) to be compatible to the lv3 BLAS.

  2. The behavior of GEMV (and GBMV) is aligned to its original proposal (https://dl.acm.org/doi/10.1145/42288.42291) that says:
    Note that it is permissible to call the routines with M or N = 0, in which case the routines exit immediately without referencing their vector or matrix arguments.
    Moreover, tests in (testBLAS) confirm that several BLAS implementations conform with this rule.

This PR proposes a fix for the issue using (2), which preserves the current behavior and is compatible to several BLAS implementations.

@codecov
Copy link

codecov bot commented May 26, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (de2b976) 0.00% compared to head (f59ece6) 0.00%.
Report is 92 commits behind head on master.

❗ Current head f59ece6 differs from pull request most recent head 658ac9d. Consider uploading reports for the commit 658ac9d to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##           master     #843     +/-   ##
=========================================
  Coverage    0.00%    0.00%             
=========================================
  Files        1908     1918     +10     
  Lines      186962   188614   +1652     
=========================================
- Misses     186962   188614   +1652     

see 146 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@angsch
Copy link
Collaborator

angsch commented Jun 13, 2023

Is this behaviour checked/enforced by some test? It is a subtle change that could slip through when vendors upgrade their LAPACK version and their implementation does not follow reference-LAPACK.

@weslleyspereira
Copy link
Collaborator Author

The most recent commit also updates *la_gbamv.f and *la_geamv.f using strategy (2).

We have:

  1. routines with m-by-n A and y: *gemv.f, *gbmv.f, *la_gbamv.f, *la_geamv.f. (8 + 4 + 4 + 4 =) 20 files. Documentation was updated.
  2. routines with n-by-n A and y: *symv.f, *spmv.f, *sbmv.f, *hemv.f, *hpmv.f, *hbmv.f, *la_syamv.f, *la_heamv.f. (4 + 4 + 6 + 2 + 2 + 2 + 4 + 2 =) 26 files. No need to update documentation.
  3. routines with m-by-n A and no y: *tbmv.f, *trmv.f, *tpmv.f. (4 + 4 + 4 =) 12 files. No need to update documentation.

@weslleyspereira
Copy link
Collaborator Author

Is this behaviour checked/enforced by some test? It is a subtle change that could slip through when vendors upgrade their LAPACK version and their implementation does not follow reference-LAPACK

Thanks for asking! Yes, we currently check this behavior in (testBLAS). In fact, multiple BLAS implementations (all libraries I tested so far in testBLAS) satisfy this behavior. We do not have such checks in the LAPACK test suite currently.

@langou langou merged commit 2983298 into Reference-LAPACK:master Aug 22, 2023
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.

Y is misscaled in ?GEMV for some cases gbmv leaves result uninitialized for zero column matrix
4 participants