search

andreww / fox

A Fortran XML library
https://andreww.github.io/fox/
Other
60 stars 51 forks source link

removed the colon for each dimension in len functions arguments. #47

Closed pietrodelugas closed 6 years ago

pietrodelugas commented 7 years ago

I don't know why but those colons were making pgi compiler produce bugged code which tried to allocate huge amount of memory anytime that a str function was called with an array argument.

andreww commented 6 years ago

I'm happy to merge this one, but could you add a comment to the top of the file outlining the problem and why the colons should not be included. It would be a good idea to give an exact version of the compiler and, if you have it, a reference for the bug report to PGI.

Out of interest, what happens for subsets of an array - e.g. s(2:4) - does this also consume massive amounts of memory?

pietrodelugas commented 6 years ago

Ok Thanks. I was not very familiar with pull requests at the time I made these actually, they were was done directly from the master branch on my fork .... If I don't find any othere way to disentangle them I will close and resubmit them appropriately. sorry

andreww commented 6 years ago

Merging from your master branch onto my master branch isn't a problem. Assuming your local master is the same as your master on github you should be able to git checkout master, add a new commit with the comments then do git push origin master. That will update this pull request with the extra comments and I'll be able to merge.

However, if your local master is ahead of your master on github it's probably easer to create a new branch and pull request from that. If you need to do this you should do is checkout your master branch (git checkout master), then create a new branch for this change (git checkout -b colons) then push that to your github fork (git push origin colons). You will then need to create a new pull request from your colons branch to my master branch. If you create a new commit on colons adding comments about the change you can push this before or after you make the pull request.

pietrodelugas commented 6 years ago

Ok Sorry I have been a little late with this one. The branch has been rebase to master. I made some tests and seems that what works for PGI compiler does not work for ifort version 12.10. Intel fixed the bug in the successive versions, at least since 13.10. I opted for using a preprocessor directive in order to make it work for ifort v 12.10 and PGI. I don't know if you prefer dropping 12.10 or keeping the preprocessor directives.

For PGI the bug is still present in latest community edition version 17.10. I don't have access to the more recent licensed versions, so I can't say if it has been fixed.

I checked the issue disappears is you pass a slice of the whole array, but if you use ia(1:dim) then the result is wrong again.

andreww commented 6 years ago

I knew this reminded me of something.

We put those colons in to work around a compiler bug in the intel compiler in 2012. See: https://github.com/andreww/fox/commit/08945882f9140d1adf59418259dacfbbed2946b2! Even given the age I suspect there are still ifort 12 users out there so using the pre-compiler seems seems sensible. I'll merge this as is.

This really makes me wish I had CI set up for intel and PGI. Both versions of the code run fine under gfortran. I suspect I can set something up with the PGI community edition. I may be able to do ifort too (waiting to see what our HPC support group discover about the small print of the licence).