example of dubious code

[roger_amos1]

The following examples can be improved to make Vasp compile correctly with a wider range of compilers.

1) in cl_shift.F line 916 (in Vasp.6.2.0)

Code: Select all

      CALL vtutor%error("CORE_WAVE_FKT: solution not found within " // str(NTRIES) // " attempts\n "&
         "n=" // str(N) // " l=" // str(L) // " j=" // str(J) // " energy interval: " // str(EMIN) // &
         " " // str(EMAX))

This depends on the order in which the compiler interprets the line. If the first thing it does is remove the '&' to produce one line, you get illegal code.
On the other hand if the first thing the compiler does is to interpret the newline \n , then you get legal code.
You can make it more robust by inserting an extra string concatenation

Code: Select all

      CALL vtutor%error("CORE_WAVE_FKT: solution not found within " // str(NTRIES) // " attempts\n " \\ &
         "n=" // str(N) // " l=" // str(L) // " j=" // str(J) // " energy interval: " // str(EMIN) // &
         " " // str(EMAX))

2) in made_fit.F line 2426

Code: Select all

    COMPLEX(q),ALLOCATABLE :: P(:),Q(:)

q (lower case) is actually a parameter defined in module prec,

Code: Select all

     INTEGER, PARAMETER :: q =SELECTED_REAL_KIND(10)

This confuses at least one compiler (the Fujitsu ARM compiler) so it would be more robust if you change the name of the variable Q (upper case)


3) in tet.F line 367 you have this,

Code: Select all

    fermiWeights(iband, kit, ispin) = &
         fermiWeights(iband, kit, ispin) + (weights(:,1) + weights(:,2))

where kit is defined as

Code: Select all

    kit => kpointIndexTetra(:, itet)

So you are updating several elements of fermiWeights simultaneously using an indexed array operation, with kpointIndexTetra as the index.

This is very dangerous and breaks the Fortran 2003/2008 standard which says you are not allowed to do that if the elements of the index repeat. You get away with this with
the Intel compilers because, if you look at how they implement this, they take the original statement and rewrite it as a loop - a serial loop not a vector one.
GFORTRAN8 gets this wrong, as does the Fujitsu Arm compiler, as would any compiler that treats the statement as the equivalent of a vector loop.

You should rewrite this as an explicit loop to prevent this. The reason it is particularly dangerous is that it compiles, but gives wrong answers.

[merzuk.kaltak]

Dear Roger,

we will test these changes as soon as we have access to a corresponding ARM machine.
Thank you for your suggestions.

[roger_amos1]

I would advise against waiting for an Arm machine to look at the third case. It is too important
Try this little program

Code: Select all

      program test
      double precision result(6),weight(4)
      integer index(4,2)
      integer n,k
      data index/1,2,5,6,1,2,3,3/
          result=0d0
          weight=1d0
          do n=1,2
          result(index(:,n))=result(index(:,n))+weight
          enddo
          write(*,*) result
      
          result=0d0
          do n=1,2
          do k=1,4
          result(index(k,n))=result(index(k,n))+weight(k)
          enddo
          enddo
          write(*,*)result
          stop
          end

and see if the first way of producing the result is the same as the second way.
The Intel compiler gives the same answer both ways, but it shouldn't!
Gfortran 8 gives different answers, which is actually correct (!) but is 'wrong' comparing it to what Vasp expects the answer to be.
The reason is that the first form breaks the Fortran 2003/2008 standard, which forbids this type of operation if the index array contains duplicate values.
The reason the Intel compiler 'works' is that it actually translates the first form into the second form internally.
You should not leave it like that, and caution your programmers against using this type of structure