Skip to content

Misc fixes#1284

Merged
langou merged 10 commits into
Reference-LAPACK:masterfrom
jschueller:cblas
Jun 11, 2026
Merged

Misc fixes#1284
langou merged 10 commits into
Reference-LAPACK:masterfrom
jschueller:cblas

Conversation

@jschueller

@jschueller jschueller commented Jun 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

random bugfixes:

@martin-frbg

martin-frbg commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Isn't #1047 already handled by PR #1264 , whose author pinged it just days ago ? Similarly, I have #1251 already open to fix #1183
And I think a mixed bag of unrelated fixes is a bad idea for other reasons as well - makes it harder to revert a single change, should that ever become necessary, and obscures the link between individual issue and fix in the git history

@jschueller

jschueller commented Jun 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

my bad, lets drop those
fixes are in separate commits (referencing the respective issues) so it can still be reverted individually

@jschueller jschueller force-pushed the cblas branch 4 times, most recently from 1a695c2 to a2f5c00 Compare June 10, 2026 14:26
@langou

langou commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

I am reviewing this pull request.

Side comment: @martin-frbg wrote "I think a mixed bag of unrelated fixes is a bad idea for other reasons as well - makes it harder to revert a single change, should that ever become necessary, and obscures the link between individual issue and fix in the git history." I agree with @martin-frbg. For future, it would be better to have PR that are more focused. I understand that there are a collection of different commits in this PR, I would prefer a collection of separated PRs. This is a comment for the future, not for this specific PR. Thanks for the PR!

langou
langou previously approved these changes Jun 11, 2026
View reviewed changes
@langou

langou commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

[ Note that I approved the whole PR by mistake, I intended to only approve a commit. ]

@langou

langou commented Jun 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

With respect to
4d7c5fa

Per LAPACK docs, in [cz]tgexc, ilst is [in,out]. The LAPACKE C interface was passing both ifst and ilst by value as input-only scalars, so callers could never observe the updated ilst value. Change both ifst and ilst to lapack_int* pointers in the complex tgexc variants to match the real-precision stgexc/dtgexc interface and the underlying Fortran semantics.

WRT ifst, the behavior is not the same between c/z (complex arithmetic case) and s/d (real arithmetic case).

In real arithmetic, there are 2-by-2 blocks, and so ifst can be changed. See:

lapack/SRC/dtgexc.f

Lines 299 to 301 in 54b6620

IF( IFST.GT.1 ) THEN
IF( A( IFST, IFST-1 ).NE.ZERO )
$ IFST = IFST - 1

There are only 1-by-1 blocks in complex arithmetic. So ifst is input only. As correctly indicated in the comments:

lapack/SRC/ztgexc.f

Line 131 in 54b6620

*> \param[in] IFST

Right now, with this PR, we have the LAPACKE interface with in/out for ifst in complex, and the comment says input only so something needs to be changed. I suggest to leave ifst as input only for LAPACKE interface. (So this is a request to change the commit.)

WRT ilst, thanks for pointing the mistake for ilst for c/z (complex arithmetic case). I agree.

ilst is in/out.

In the complex case, we only have 1-by-1 blocks but the reordering can fail and then ilst is used to return when the error happen. (I would prefer this to be indicated in INFO but so be it.)

In the real case, we can have an error in the reordering and we also have the issue of 1-by-1 and 2-by-2 blocks which means that, even w/o errors, ilst is only known +/-1 and so the final value is returned by the function.

I repeat the request: please leave ifst as input for [c/z] in LAPACKE.

@langou

langou commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

With respect to
4d7c5fa
I am not sure we are ready for testing/ in LAPACKE. I agree that we should have something. I do not know that what this PR proposes is what we want. I do not know what we want. LAPACKE is tested by the test suite in LAPACK++, and maybe this is how we want to go about it. I suggest to remove the testing/ bit for now.

@langou

langou commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

With respect to:
3078fea

Can you please propagate the same changes from [c] to [s,c,z]?

@langou

langou commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Well, reviewing the PR, and going commit by commit was actually not too bad. It kind of worked on my end and I was worried about the mix-and-match of commits. At the end, this worked OK on my end.

There are two requests for change.

  1. please leave ifst as input for [c/z] in LAPACKE for [cz]tgexc
  2. remove testing/ from LAPACKE.

Otherwise, I am done reviewing the PR. I am ready to merge.

This is a terrific amount of work, it fixes a lots of issues, thanks a lot.

@jschueller
cblas_dgemmtr: Fix syntax error
ea7f510
@jschueller
cblas_zgemmtr: Fix routine name
8a43b68
@jschueller
lapacke_zgedmdq: fix routine name
d296703
@jschueller
lapacke: fix memory allocation on error
7a721b3
@jschueller
cblas_ctrmm: fix error check
fd16118 
Closes Reference-LAPACK#1188
@jschueller
LAPACKE: Make LDU/LDVT checks in GESVD conditional on JOBU/JOBVT
3d668fe 
When JOBU='N' or JOBVT='N', the U and VT matrices are not referenced,
so the leading dimension checks should be skipped.  Previously the
code rejected ldvt=0 (and ldu=0) even when those matrices were not
used, causing a spurious INFO=12 error on row-major calls with
JOBVT='N' (or INFO=10 for JOBU='N').

Fixes Reference-LAPACK#1090
@jschueller

jschueller commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

ok, done

langou
langou reviewed Jun 11, 2026
View reviewed changes
Comment thread LAPACKE/include/lapacke.h Outdated
langou
langou reviewed Jun 11, 2026
View reviewed changes
Comment thread LAPACKE/include/lapacke_64.h Outdated
Comment thread LAPACKE/src/lapacke_ztgexc_work.c Outdated
@jschueller

jschueller commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

yes, fixed the work functions

@jschueller
Fix mislabeled comments in dget38.f
526a4fb 
Two comments in the DGEHRD/DHSEQR test incorrectly say "Compute
Schur form":
- The DGEHRD call reduces to upper Hessenberg form, not Schur.
- The DHSEQR call computes eigenvalues and the Schur form, so
  use a more descriptive label.

Fixes Reference-LAPACK#587
@jschueller
Fix DROTMG/SROTMG rescaling overwriting DH12
6dc1a78 
When DFLAG=0 and both DD1 and DD2 need rescaling, the first
scaling loop transitions DFLAG from 0 to -1 and correctly
scales DH11 and DH12. But the second scaling loop then hits
the ELSE branch (matching DFLAG=-1) which unconditionally
resets DH21=-1 and DH12=1, overwriting the scaled DH12.

Change ELSE to ELSE IF (DFLAG.EQ.ONE) so the implied-element
initialization (DH21=-1, DH12=1) only fires when DFLAG=1,
not when DFLAG=-1 (where all elements are already explicit).

Fixes Reference-LAPACK#244
@jschueller
Guard ?GEEV/?GEEVX undo-scaling against negative INFO
91c0f61 
When ?HSEQR encounters non-finite input, it can return a negative
INFO. The undo-scaling block at label 50 then computes N-INFO (which
exceeds N) and indexes WR( INFO+1 ) (out of bounds), causing memory
corruption through ?LASCL.

Add INFO.GE.0 to the outer IF( SCALEA ) guard so the undo-scaling
block is skipped entirely when INFO is negative. The inner
IF( INFO.GT.0 ) guard only covered the second pair of ?LASCL calls.

Fixes Reference-LAPACK#1128
@jschueller
Make LAPACKE_[cz]tgexc pass ifst/ilst as pointer outputs
283cf5b 
Per LAPACK docs, in [cz]tgexc, ilst is [in,out]. The LAPACKE C
interface was passing both ifst and ilst by value as input-only
scalars, so callers could never observe the updated ilst value.

Change both ifst and ilst to lapack_int* pointers in the complex
tgexc variants to match the real-precision stgexc/dtgexc interface
and the underlying Fortran semantics.

Fixes Reference-LAPACK#771
@martin-frbg

martin-frbg commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

Umm, can you please update the PR text before this gets merged ? I get the impression you added another bunch of valuable fixes in the latest round of changes ?

@jschueller

jschueller commented Jun 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

no, I did not add commits, I just rebased to master, I had 12 then removed the 2 commits duplicating other PRs

@martin-frbg

martin-frbg commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

no, I did not add commits, I just rebased to master, I had 12 then removed the 2 commits duplicating other PRs

Oh, ok - thought the gemmtr test fixes and lapacke_zgedmdq rename were new (guess it illustrates how I have comprehension issues with mixed pickles fixes)

@langou langou merged commit 077667a into Reference-LAPACK:master Jun 11, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@langou langou langou approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jschueller @martin-frbg @langou