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

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

Merged
merged 3 commits into from
Apr 10, 2018

Conversation

pietrodelugas
Copy link

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
Copy link
Owner

andreww commented Apr 3, 2018

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
Copy link
Author

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
Copy link
Owner

andreww commented Apr 5, 2018

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.

…as making pgi compilers produce bugged code which tried to allocate huge amount of memory anytime that a str function was called with an array argument
@pietrodelugas
Copy link
Author

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
Copy link
Owner

andreww commented Apr 10, 2018

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: 0894588! 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).

This was referenced Apr 10, 2018
@andreww andreww merged commit bc2b011 into andreww:master Apr 10, 2018
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.

2 participants