Try implicit none

[weslleyspereira]

Closes #484

Following @thijssteel's suggestion, I added -fimplicit-none to the CI.

[weslleyspereira]

Does someone know how to avoid explicitly setting up ulimit -s 16384 ?

[thijssteel]

i have no idea how to avoid setting ulimit.

This PR might be a good opportunity to add -fcheck=bounds to the ci as well.

[weslleyspereira]

i have no idea how to avoid setting ulimit.

This PR might be a good opportunity to add -fcheck=bounds to the ci as well.

I found four out of bound errors when running the tests with the flag. I will try to look at them tomorrow, but maybe it is better to address this issue in another branch. I don't know...

[codecov]

Codecov Report

Merging #486 (d654f4b) into master (d62305e) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #486      +/-   ##
==========================================
- Coverage   83.34%   83.33%   -0.01%     
==========================================
  Files        1820     1820              
  Lines      170849   170849              
==========================================
- Hits       142395   142378      -17     
- Misses      28454    28471      +17     

Impacted Files	Coverage Δ
SRC/slahqr.f	91.62% <0.00%> (-2.80%)	⬇️
SRC/strevc.f	81.72% <0.00%> (-2.09%)	⬇️
SRC/slaein.f	80.28% <0.00%> (-1.41%)	⬇️
SRC/clahqr.f	95.67% <0.00%> (-1.24%)	⬇️
SRC/claqr0.f	86.66% <0.00%> (+0.74%)	⬆️

---

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 d62305e...d654f4b. Read the comment docs.

[thijssteel]

bounds checks were addressed in #481, i guess it can wait till that routine is fixed

[martin-frbg]

Does someone know how to avoid explicitly setting up ulimit -s 16384 ?

I wonder if it would be acceptable to build just TESTING/EIG (or in particular, xeigtstz) without -frecursive ?

[weslleyspereira]

Does someone know how to avoid explicitly setting up ulimit -s 16384 ?

I wonder if it would be acceptable to build just TESTING/EIG (or in particular, xeigtstz) without -frecursive ?

Yes.. maybe, since it is optional. I don't have formed opinion about that. I will mention Julien @langou here to give his opinion.

[langou]

Yes, sure, if that fixes it, compiling the .o in EIG related to "xeigtstz" without -frecursive flag is fine. I am confused. I thought -frecursive was supposed actually to help with the ulimit -s 16384.

[thijssteel]

-frecursive forces stack allocation, so it doesn't surprize me that it causes the limit of the stack size to be reached.

also, i think -frecursive is supposed to be more of a bandaid, needed because not all routines are correctly marked as recursive.

[langou]

also, i think -frecursive is supposed to be more of a bandaid, needed because not all routines are correctly marked as recursive.

=> no no, I do not think this is why we used this flag. I think all recursive routines are correctly labelled. We used the flag -frecursive as bandaid indeed. I forgot what the scratch was though. Well, shall we try to remove the flag then? If we have a few RECURSIVE routines not labelled correctly, we should probably label them correctly anyway. And it seems that I forgot why we added the flag in the Makefile in the first place. It was bandaid for sure.

-frecursive forces stack allocation, so it doesn't surprize me that it causes the limit of the stack size to be reached.

=> Thanks for explaining.

[weslleyspereira]

Ok then! I will try it here and see if Travis is ok without ulimit

[thijssteel]

A possible explanation is multithreading. If multiple threads call a routine with static allocations it might interfere. Stack allocations would avoid that. But its fine for the ci since it doesn't do multithreading (at least not within the same process).

And for reference, the reason that martin suggested excluding the eigenvalue tests is because those require a pretty large local workspace for legacy reasons. This is partly so that the small bulge multishift qr implementation could be a drop in replacement for the old one. For zhseqr, a 49x49 double complex local array is easily larger than system stack limits. To my knowledge, this is the largest local array in the entire repository.

[zerothi]

Please see details about the frecursive flag here #188 , I don't know if they are fixed yet.
Also info may be found here

[langou]

A possible explanation is multithreading. If multiple threads call a routine with static allocations it might interfere. Stack allocations would avoid that. But its fine for the ci since it doesn't do multithreading (at least not within the same process).

=> Ah yes, that is that, thanks @thijssteel.

And @zerothi is correct in reminding us of
https://stackoverflow.com/questions/18216314/shouldnt-lapacks-dsyevr-function-for-eigenvalues-and-eigenvectors-be-thread-s

So, based on this, I think we want to leave the -frecursive flag in the make.inc.example of LAPACK. And try to use the -frecursive flag as much as possible, if not all the time. I do not think we should remove the -frecursive flag.

[thijssteel]

ugh, this is why we can't have nice things.

On a more serious note, i disagree that we should use the flag as much as possible.
We should make the CI as strict as possible. So that it can find as many problems as possible.
If there is a routine that should be marked as recursive but isn't. I want to know about it.
But the general purpose make should have as many safeguards as possible so that end users do not have problems.

For the same reason, i think we should add implicit none to the ci, but not to the general make.
If someone merges a faulty PR by accident, we don't want to master branch of a repo like this to be unbuildable.

[langou]

Hi @thijssteel, I think we agree. I agree with most you write above.

On a more serious note, i disagree that we should use the flag as much as possible.

What I was trying to say is that we want to avoid as much as possible users to create a libreflapack.a that is not thread safe. So I think we want to have -frecursive in make.inc.example. Julien.

[thijssteel]

As a sidenote, since my recent PR, we can actually reduce NL a little if we want to. NL=27 now allows for 4 simultanious shifts and a 13x13 aed window. Should be plenty for such a rare case.

[weslleyspereira]

Based on the previous comments, I think we all agree with #486 (comment)
I reverted be1e324 so that the PR agrees with that too. Thanks you all!

[zerothi]

On a more serious note, i disagree that we should use the flag as much as possible.
We should make the CI as strict as possible. So that it can find as many problems as possible.
If there is a routine that should be marked as recursive but isn't. I want to know about it.
But the general purpose make should have as many safeguards as possible so that end users do not have problems.

I agree! I removed it then, which was a mistake. So work-arounds or implicit recursive declarations would be nice in these cases.

[weslleyspereira]

Summary of PR #486:

1. There was a bug in the Makefile, and this is why AppVeyor stopped working. I solved it.
2. A recent commit was caused by a variable that was not defined, and the compiler couldn't track it. @thijssteel suggested adding -fimplicit-none to the CI on Implicit typing #484. I did it.

[martin-frbg]

Just one last question regarding the -frecursive - and it may be a silly one, but how are cmake builds expected to receive this compiler option ? Are we just relying on people picking up the suggestions from the various make.inc examples (that in themselves are irrelevant to cmake), and copying them to their FFLAGS or CMAKE_Fortran_FLAGS ?