[BLAS] Fix missing logic path in xrotmg.f

[timleslie]

This fixes a bug in SROTMG and DROTMG where there is a missing code path.

I traced the bug back to the following commit: 1767365#diff-17bc8cba27cfa755d1503dee5651235a35de951b677ecaa5f3adb596137005a2

In particular, the original code had:

  IF (.NOT.DU.LE.ZERO) GO TO 30
*         GO ZERO-H-D-AND-DX1..
  GO TO 60

The code at 60 provided the ZERO-H-D-AND-DX1 operations, which is what was lost in the referenced commit. This PR puts this code path back in.

It's worth noting that according to file:///Users/timl/Downloads/Restructuring_the_BLAS_Level_1_Routine_for_Computi.pdf (bottom of page 7) this code path may not actually be reachable, so this change is possibly just for completeness, it may not fix any real world cases.

[codecov]

Codecov Report

Merging #455 into master will increase coverage by 1.38%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #455      +/-   ##
==========================================
+ Coverage   81.86%   83.25%   +1.38%     
==========================================
  Files        1863     1808      -55     
  Lines      181096   170263   -10833     
==========================================
- Hits       148263   141747    -6516     
+ Misses      32833    28516    -4317     

Impacted Files	Coverage Δ
SRC/dlat2s.f	76.47% <0.00%> (-4.49%)	⬇️
SRC/dtgexc.f	46.35% <0.00%> (-3.98%)	⬇️
SRC/dlansb.f	62.50% <0.00%> (-3.29%)	⬇️
SRC/slansb.f	62.50% <0.00%> (-3.29%)	⬇️
SRC/clansy.f	65.51% <0.00%> (-3.06%)	⬇️
SRC/zlansy.f	65.51% <0.00%> (-3.06%)	⬇️
SRC/clangb.f	72.50% <0.00%> (-3.02%)	⬇️
SRC/dlangb.f	72.50% <0.00%> (-3.02%)	⬇️
SRC/slangb.f	72.50% <0.00%> (-3.02%)	⬇️
SRC/zlangb.f	72.50% <0.00%> (-3.02%)	⬇️
... and 1185 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d1afcd...6829744. Read the comment docs.

[martin-frbg]

Hmm - if a code path is unreachable and was removed by a cleanup, I am not sure it should be put back "for completeness" unless there is evidence that it is actually reachable ? (At the very least, nobody appears to have missed it since its removal, 9 years ago ?)

[langou]

Hi @timleslie, you write: This fixes a bug in SROTMG and DROTMG. Can you explain the bug that is fixed? How you discovered the bug, etc.? It would be good for many reasons that we understand what these few lines of codes. I think we must have SU >=0 and so it is possible that SU=0 in some cases. And actually we might have SU < 0. Is that what is going on? And so this is a piece of code that handles the rare case when SU <=0. (So we are essentially assuming that SU = 0 in this case. Which sounds good to me.) @timleslie: can you explain a tat? Cheers, Julien.

[timleslie]

Here's the correct link to the paper I referenced rather than a local copy 😅 https://www.researchgate.net/publication/2388100_Restructuring_the_BLAS_Level_1_Routine_for_Computing_the_Modified_Givens_Transformation

Hmm - if a code path is unreachable and was removed by a cleanup, I am not sure it should be put back "for completeness" unless there is evidence that it is actually reachable ? (At the very least, nobody appears to have missed it since its removal, 9 years ago ?)

Yep, I think you're probably right. if the code path isn't reachable then the alternate change here would be to simply remove the check of IF (DU.GT.ZERO) THEN.... My concern is that either a) the check is necessary, in which case we have a missing code path or b) the check isn't necessary, in which case we have an extra check that can be removed.

I don't have any evidence that the check is necessary, but I also haven't run any experiments to try to find a set of values which can trigger the code path.

Hi @timleslie, you write: This fixes a bug in SROTMG and DROTMG. Can you explain the bug that is fixed? How you discovered the bug, etc.? It would be good for many reasons that we understand what these few lines of codes. I think we must have SU >=0 and so it is possible that SU=0 in some cases. And actually we might have SU < 0. Is that what is going on? And so this is a piece of code that handles the rare case when SU <=0. (So we are essentially assuming that SU = 0 in this case. Which sounds good to me.) @timleslie: can you explain a tat? Cheers, Julien.

Calling it a bug is perhaps overstating the situation. The potential bug in this case is that if we have SU <= 0 then the value of SFLAG and the other SPARAMS are never set. I'm not sure how different compilers will handle this but I suspect it would result in undefined behaviour.

As noted above, this is just a potential bug, I don't have any evidence yet that indicates it can possibly be triggered. This extract from linked article notes that it would require some very specific rounding conditions to exist.

That said, if we think there is an outside chance that the condition could be met then the code should account for it explicitly.

I found this issue by inspection of the code. I'm trying to gain a deep understanding of the BLAS library for my own interest and I noticed this missing logic path.

The algorithm involved is best described here (see Appendix): https://www.researchgate.net/publication/220492248_Basic_linear_algebra_subprograms_for_FORTRAN_usage

It notes that 0 < u <= 2 should hold true, which implies that the check is actually unnecessary.

[langou]

Thanks @timleslie for explaining. This helps a lot. So we have a condition that we do not think will ever happen. (Per the paper that you quote.) And the current code has a conditional IF ( something that we think we always be true ) . . . ENDIF. And you are arguing that we should either remove the conditional or have an ELSE statement just in case. Thanks for explaining. And refreshing my memory with the paper of Tim Hopkins. Okey doc, so my opinion is that we should accept your PR. I would be in favor to have a quick comment just after the ELSE to explain that this ELSE is protective coding and we do not think we should enter the ELSE statement.

Note: I was surprised to see that the CODECOV report did not report a decrease in code coverage. This PR should decrease the code coverage since we now have some line of codes that our test suite is not entering. Maybe SROTMG and DROTMG are not tested in our test suite.

[timleslie]

Okey doc, so my opinion is that we should accept your PR. I would be in favor to have a quick comment just after the ELSE to explain that this ELSE is protective coding and we do not think we should enter the ELSE statement.

Yep, I think that sounds good, I'll add that comment and add a reference to the Hopkins paper for the next person who encounters this code.

Note: I was surprised to see that the CODECOV report did not report a decrease in code coverage. This PR should decrease the code coverage since we now have some line of codes that our test suite is not entering. Maybe SROTMG and DROTMG are not tested in our test suite.

I'm not yet familiar enough with the test suite to say for sure, but I agree that it makes sense that the coverage would decrease with this change.