[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

Currently, I cannot test the option `-fconvert=big-endian` with open statement with convert argument using the following code since lowering is not supported. I am not sure if it can be tested in runtime.

  $ cat fconvert_option_openfile.f90 
  module fconvert_option_openfile
  
  contains
    subroutine open_for_read(fid, file_name)
      integer :: fid
      character(:), allocatable :: file_name
      open (fid, file=file_name, form='UNFORMATTED', status='old')
    end
    subroutine open_for_read_BE(fid, file_name)
      integer :: fid
      character(:), allocatable :: file_name
      open (fid, file=file_name, form='UNFORMATTED', status='old', convert="BIG_ENDIAN")
    end
    subroutine open_for_read_LE(fid, file_name)
      integer :: fid
      character(:), allocatable :: file_name
      open (fid, file=file_name, form='UNFORMATTED', status='old', convert="LITTLE_ENDIAN")
    end
    subroutine open_for_read_native(fid, file_name)
      integer :: fid
      character(:), allocatable :: file_name
      open (fid, file=file_name, form='UNFORMATTED', status='old', convert="NATIVE")
    end
  
    subroutine open_for_write(fid, file_name)
      integer :: fid
      character(:), allocatable :: file_name
      open (fid, file=file_name, form='UNFORMATTED', status='new')
    end
    subroutine open_for_write_BE(fid, file_name)
      integer :: fid
      character(:), allocatable :: file_name
      open (fid, file=file_name, form='UNFORMATTED', status='new', convert="BIG_ENDIAN")
    end
    subroutine open_for_write_LE(fid, file_name)
      integer :: fid
      character(:), allocatable :: file_name
      open (fid, file=file_name, form='UNFORMATTED', status='new', convert="LITTLE_ENDIAN")
    end
    subroutine open_for_write_native(fid, file_name)
      integer :: fid
      character(:), allocatable :: file_name
      open (fid, file=file_name, form='UNFORMATTED', status='new', convert="NATIVE")
    end
  end
  $ cat fconvert_option_readfile.f90 
  module fconvert_option_readfile
  
  contains
    subroutine readfile(fid, del_flag, expect, n, t)
      integer :: n, t
      integer :: fid, del_flag
      real :: expect(n)
      real :: buf(n)
  
      read (fid) buf
      if (del_flag .eq. 0) then
        close (fid)
      else
        close (fid, status='delete')
      end if
  
      do i = 1, n
        if (buf(i) .ne. expect(i)) stop (i+t)
      enddo
    end
  end
  $ cat fconvert_option_main_1.f90 
  program fconvert_option_main_1
    use fconvert_option_openfile
    use fconvert_option_readfile
  
    integer, parameter :: n = 4
    integer :: del_flag = 0 ! 1 for deleting data file after read
    real :: arr(n) = [1.0, 2.0, 3.0, 4.0]
    character(:), allocatable :: filename
    integer :: arglen
  
    call get_command_argument(1, length = arglen)
    allocate(character(arglen) :: filename)
    call get_command_argument(1, value = filename)
  
    call open_for_read(10, filename)
    call readfile(10, del_flag, arr, n, 0)
  
    call open_for_read_LE(11, filename)
    call readfile(11, del_flag, arr, n, 4)
  
    call open_for_read_native(12, filename)
    call readfile(12, del_flag, arr, n, 8)
  
    print *, 'PASS'
  end

BTW, can you continue working on the lowering of the convert argument of open statement?



================
Comment at: clang/include/clang/Driver/Options.td:4897
 def ffixed_line_length_VALUE : Joined<["-"], "ffixed-line-length-">, Group<f_Group>, Alias<ffixed_line_length_EQ>;
+def fconvert_EQ : Joined<["-"], "fconvert=">, Group<f_Group>,
+  HelpText<"Set endian conversion of data for unformatted files">;
----------------
awarzynski wrote:
> jpenix-quic wrote:
> > peixin wrote:
> > > Why do you move it here? Maybe it is not implemented now, clang may need this option eventually. @MaskRay 
> > I was using the fixed line length options as a reference for how to handle this--based on the discussion in the review here (https://reviews.llvm.org/D95460) about forwarding options to gfortran, I was thinking that it would also be safe to handle fconvert similarly, but I'm not 100% sure and definitely might be misunderstanding something!
> GFortran support has been effectively broken since LLVM 12 (see e.g. this [[ https://github.com/llvm/llvm-project/commit/6a75496836ea14bcfd2f4b59d35a1cad4ac58cee | change ]]). We would do ourselves a favor if we just removed it altogether (I'm not aware of anyone requiring  it). 
> 
> And if Clang ever needs this option, we can always update this definition accordingly. No need to optimize for hypothetical scenarios that may or may not happen :) To me, this change makes perfect sense.
OK. This sounds reasonable to me.


================
Comment at: flang/lib/Lower/Bridge.cpp:2757
 
+    if (funit.isMainProgram()) {
+      auto conversion = bridge.getConversion();
----------------
I think this should be moved into `void lowerFunc(Fortran::lower::pft::FunctionLikeUnit &funit)`.


================
Comment at: flang/runtime/main.cpp:55
+
+  if (auto convert{Fortran::runtime::GetConvertFromInt(i)}) {
+    Fortran::runtime::executionEnvironment.conversion = *convert;
----------------
Nit: Is it better to to check the range of i before getting it from `GetConvertFromInt`?
```
if (i < static_cast<int>(Convert::Unknown) || i > static_cast<int>(Convert::Swap)) {
  crash
} else {
  Fortran::runtime::executionEnvironment.conversion = static_cast<Convert>(i);
}
```


================
Comment at: flang/runtime/main.cpp:58
+  } else {
+    Fortran::runtime::Terminator{__FILE__, __LINE__}.Crash(
+        "Invalid convert option passed to ConvertOption");
----------------
Should `__FILE__, __LINE__` be passed as argument in lowering to point to the file name and line of the source file? Or is this (__FILE__, __LINE__) pointing the the file name and line of the source file?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130513/new/

https://reviews.llvm.org/D130513