This is an archive of the discontinued LLVM Phabricator instance.

[Flang] Add -fconvert option to swap endianness for unformatted files
ClosedPublic

Authored by jpenix-quic on Jul 25 2022, 1:16 PM.

Details

Summary

To accomplish this, this patch creates an optional list of environment
variable default values to be set by the runtime to allow directly using
the existing runtime implementation of FORT_CONVERT for I/O conversions.

Resolves issue: https://github.com/llvm/llvm-project/issues/55961

Diff Detail

Event Timeline

jpenix-quic created this revision.Jul 25 2022, 1:16 PM
Herald added a project: Restricted Project. · View Herald Transcript
jpenix-quic requested review of this revision.Jul 25 2022, 1:16 PM
Herald added a project: Restricted Project. · View Herald Transcript

I do have a few related lingering questions as well:

  1. While this implementation mimics gfortran's behavior, there are still differences with other compilers (please see the comment here: https://github.com/llvm/llvm-project/issues/55961#issuecomment-1175677659). Is there a good place to document this (if worthwhile)?
  2. There are a few differences in how gfortran and this implementation prioritize the convert command line option, environment variable, and CONVERT= specifier (please see comment here https://github.com/llvm/llvm-project/issues/55961#issuecomment-1172547132). Would this be worthwhile to change to match gfortran as well? If not, is there maybe a good place to document this?
klausler added a comment.EditedJul 25 2022, 1:28 PM

Instead of adding new custom APIs that let command-line options control behavior in a way that is redundant with the runtime environment, I suggest that you try a more general runtime library API by which the main program can specify a default environment variable setting, or a set of them. Then turn the command-line options into the equivalent environment settings and pass them as default settings that could be overridden by the actual environment.

This would not be any more work, it would lead to a cleaner implementation in the runtime than this one, and it would make the next option of this kind much easier to implement.

klausler added inline comments.Jul 25 2022, 1:31 PM
flang/runtime/main.cpp
53

We always use braces on if/for/while/&c. constructs in the runtime.

Thank you for taking a look at this and thank you for the feedback!

I'm not sure if I am entirely following/I think I am confusing myself on some of the specifics. My understanding of your suggestion is that, rather than add and create a call to a fconvert-specific runtime function (ConvertOption) I should look towards implementing and calling a runtime function/API that (conceptually) looks something like SetRuntimeDefaults(FORT_CONVERT="SWAP", [...]) to set whatever was specified on the command line.

I think where I'm getting confused is when you mention default environment variable settings or translating the command-line options into the equivalent environment settings and passing them as defaults: is the idea to set the actual (in this case) FORT_CONVERT environment variable if it hasn't already been defined in the environment, with the goal of letting the existing FORT_CONVERT handling deal with everything? Or, is directly setting the executionEnvironment.conversion value like I tried to do in ConvertOption ok, but I need to rethink the ConvertOption API itself? If the goal is to set FORT_CONVERT, I'm getting a bit hung up on FORT_CONVERT currently being handled as part of the "hardcoded" main in Fort_main.c.

peixin added inline comments.Jul 25 2022, 6:57 PM
clang/include/clang/Driver/Options.td
4938

Why do you move it here? Maybe it is not implemented now, clang may need this option eventually. @MaskRay

jpenix-quic added inline comments.Jul 25 2022, 10:58 PM
clang/include/clang/Driver/Options.td
4938

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!

clang/lib/Driver/ToolChains/Flang.cpp
64

Reading through https://reviews.llvm.org/D95460 again, I'm not sure this is the appropriate place to add . I am marking this as a TODO that I will revisit with the other feedback!

Thanks for working on this. This is not my area of expertise, so I focused on the implementation in the driver.

clang/include/clang/Driver/Options.td
4938

GFortran support has been effectively broken since LLVM 12 (see e.g. this 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.

clang/lib/Driver/ToolChains/Flang.cpp
64

You can use AddOtherOptions instead. AddFortranDialectOptions is more about language options. Is this a language option? It's a bit of a borderline. Feel free to add another hook too.

Btw, the reformatting is an unrelated change. Could you submit in a separate patch? Thanks!

Added changes to address feedback about missing braces and where I am adding options::OPT_fconvert_EQ (and removing the extra formatting change). Also removes an unnecessary include I had mistakenly added.

jpenix-quic marked 2 inline comments as done.Jul 26 2022, 3:03 PM
jpenix-quic added inline comments.
clang/lib/Driver/ToolChains/Flang.cpp
64

I went the AddOtherOptions route for now. Apologies for the unrelated change and thank you for checking over this!

jpenix-quic marked an inline comment as done.Jul 26 2022, 3:07 PM

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?

clang/include/clang/Driver/Options.td
4938

OK. This sounds reasonable to me.

flang/lib/Lower/Bridge.cpp
2902

I think this should be moved into void lowerFunc(Fortran::lower::pft::FunctionLikeUnit &funit).

flang/runtime/main.cpp
57

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);
}
60

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?

Attempted to address comments suggesting a different approach for the implementation. Rather than add a call to a new set_convert() runtime function, create a list of environment variable defaults that is passed into and set by the runtime via a "known" extern data structure. Also rebased.

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

I will take a look at this!

flang/lib/Lower/Bridge.cpp
2902

I ended moving this again while changing approaches--the equivalent line is above at lines 243-252. Does the new location seem reasonable? I had to add the bool to indirectly track if a main program was specified, but the new location seemed the most fitting to me.

flang/runtime/main.cpp
57

I ended up not needing GetConvertFromInt when reworking the approach, so this has been removed!

60

I removed this instance of __FILE__, __LINE__, but I added another similar one in environment.cpp line 33!

The error points to environment.cpp line 33 in the new revision. I'm not sure if this is the best way of handling it, but given that there isn't a Fortran source line associated with either the environment list or the old runtime call (and as I think both are closer to implementation details than user-facing features), pointing to the runtime file seemed like the least confusing option for the error location.

Hopefully fix Window's build errors by using the proper Windows-specific environment utilities (_putenv_s vs setenv).

peixin added inline comments.Aug 4 2022, 2:21 AM
flang/lib/Lower/Bridge.cpp
228

Doing this can avoid add one variable to the bridge.

jpenix-quic edited the summary of this revision. (Show Details)

Change hasMainProgram to be a local variable and update summary to reflect the new approach.

jpenix-quic added inline comments.Aug 8 2022, 12:06 PM
flang/lib/Lower/Bridge.cpp
228

Done! (Although, I added the isMainProgram() check/update to the lambda below as it is a member function of FunctionLikeUnit specifically--please let me know if this looks ok!)

peixin added a comment.Aug 8 2022, 6:27 PM

This needs rebase.

flang/lib/Lower/Bridge.cpp
228

Yes.

Rebase to address conflicts, use string substitution blocks in my tests.

@klausler Could you please take a look at this again and let me know if it is more in line with your suggestion above?

I can't speak to the lowering or driver bits, but the runtime part looks pretty good to me.

flang/runtime/environment-default-list.h
2

If you want this header to be maximally portable C and C++, please observe the usage of other such headers, and use old-school C /*comments*/.

You could probably just use an 'int' for the item count and avoid some difficulty below, unless you expect a program to use billions of default environment settings.

This needs rebase. I failed to apply this patch due to conflict.

jpenix-quic updated this revision to Diff 455975.EditedAug 26 2022, 12:00 PM

Fixed up/simplified environment-default-list.h (and associated tests/lowering) for C/C++ compatibility. Also cleaned up a declaration and a few autos in the lowering component. Rebased.

jpenix-quic added inline comments.Aug 26 2022, 12:27 PM
flang/runtime/environment-default-list.h
2

Done re: using an int!

Just to double check regarding C/C++ portability and looking at other headers, one that I was looking at was Decimal/decimal.h and the structs, etc. in that file are conditionally added to namespaces depending on whether it is C or C++. I was waffling on whether I should be doing that here though (I am not currently/was not previously) as keeping the type out of the namespace allows me to keep a consistent type/directly pass the pointer from Fortran_main.c to main.cpp/enviornment.cpp. But, as a result I'm also polluting the default namespace with EnvironmentDefaultList/Item for C++ code. Is how I have it currently ok, or would it be better to move the structs into namespaces for C++ and just cast to the correct type along the way? (Or, is there another option I am missing?)

peixin added a comment.EditedAug 26 2022, 8:24 PM
$ cat fconvert_option_main_2.f90 
!
! Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
! See https://llvm.org/LICENSE.txt for license information.
! SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
!
! checking for I/O: testing read with -fconvert=big-endian.
! The convert argument of open function has prior claim over fconvert option.
! Only main program intentionally compiled with -fconvert=big-endian.

program fconvert_option_main_2
  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_LE(10, filename)
  call readfile(10, del_flag, arr, n, 0)

  call open_for_read_native(11, filename)
  call readfile(11, del_flag, arr, n, 4)

  print *, 'PASS'
end
$ cat fconvert_option_readfile.f90 
!
! Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
! See https://llvm.org/LICENSE.txt for license information.
! SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
!
! checking for I/O: readfile subroutine

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_openfile.f90 
!
! Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
! See https://llvm.org/LICENSE.txt for license information.
! SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
!
! checking for I/O: openfile subroutines

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_writefile.f90 
!
! Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
! See https://llvm.org/LICENSE.txt for license information.
! SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
!
! checking for I/O: writefile subroutine

module fconvert_option_writefile

contains
  subroutine writefile(fid, buf, n)
    integer :: n
    real :: buf(n)
    integer :: fid

    write (fid) buf
    close (fid)
  end
end
$ cat run.sh 
#!/bin/bash

echo "---------- gfortran ----------"
gfortran fconvert_option_readfile.f90 -c
gfortran fconvert_option_openfile.f90 -c
gfortran fconvert_option_writefile.f90 -c
gfortran $1.f90 $2 -c
gfortran $1.o fconvert_option_readfile.o fconvert_option_openfile.o fconvert_option_writefile.o
./a.out Inputs/fc_opt_data_$3 && rm *.o *.mod a.out

echo && echo "---------- flang-new ----------"
flang-new fconvert_option_readfile.f90 -c
flang-new fconvert_option_openfile.f90 -c
flang-new fconvert_option_writefile.f90 -c
flang-new $1.f90 $2 -c
flang-new -flang-experimental-exec $1.o fconvert_option_readfile.o fconvert_option_openfile.o fconvert_option_writefile.o 
./a.out Inputs/fc_opt_data_$3 && rm *.o *.mod a.out

$ bash run.sh fconvert_option_main_2 -fconvert=big-endian little
---------- gfortran ----------
 PASS

---------- flang-new ----------

fatal Fortran runtime error(./fconvert_option_readfile.f90:17): Unformatted variable-length sequential file input failed at record #1 (file offset 0): hit EOF reading record with length 268435456 bytes
run.sh: line 17: 126614 Aborted                 (core dumped) ./a.out Inputs/fc_opt_data_$3

Not sure if this fail is caused by this patch or runtime IO functions. I will add our other internal tests to https://github.com/llvm/llvm-project/issues/55961 later and you can also test those cases. The data is an array of "1.0 2.0 3.0 4.0" through little-endian or big-endian. You can create it by yourself.

peixin added a comment.Sep 2 2022, 9:16 AM

Mostly LGTM. Have several more comments.

flang/lib/Lower/Bridge.cpp
274

Nit

flang/lib/Optimizer/Builder/Runtime/EnvironmentDefaults.cpp
20
if (builder.getNamedGlobal(envDefaultListPtrName))
  return;

I don't think this and doEnvironmentDefaultList are necessary.

flang/test/Driver/emit-mlir.f90
16

Is it possible not to generated this global variable if fconvert= is not specified?

jpenix-quic added inline comments.Sep 2 2022, 11:59 AM
flang/test/Driver/emit-mlir.f90
16

I'm not entirely sure--the issue I was running into was how to handle this in Fortran_main.c in a way which worked for all of GCC/Clang/Visual Studio (and maybe others?). I was originally thinking of doing this by using a weak definition of _QQEnvironmentDefaults set to nullptr so fconvert, etc. could override this definition without explicitly generating the fallback case. For GCC/clang, I think I could use attribute((weak)), but I wasn't sure how to handle this if someone tried to build with Visual Studio (or maybe another toolchain). I saw a few workarounds (ex: https://devblogs.microsoft.com/oldnewthing/20200731-00/?p=104024) but I shied away from this since it seems to be an undocumented feature (and presumably only helps with Visual Studio).

Do you know of a better or more general way I could do this? (Or, is there non-weak symbol approach that might be better that I'm missing?)

peixin added inline comments.Sep 3 2022, 7:20 AM
flang/runtime/environment.cpp
77

What is environ used for? Should we try to keep as less extern variable as possible in runtime for security.

flang/test/Driver/emit-mlir.f90
16

How about generate one runtime function with the argument of EnvironmentDefaultList? This will avoid this and using one extern variable?

If users use one variable with bind C name _QQEnvironmentDefaults in fortran or one variable with name _QQEnvironmentDefaults in C, it is risky. Would using the runtime function and static variable with the type EnvironmentDefaultList in runtime be safer?

Remove unneeded braces and unnecessary changes to NameUniquer.

jpenix-quic marked 2 inline comments as done.Sep 16 2022, 11:43 AM
jpenix-quic added inline comments.
flang/runtime/environment.cpp
77

I think setenv is only required to make sure that the environ pointer points to the correct copy of the environment variables. So, as long as we are storing a copy of the environment pointer in ExecutionEnvironment and potentially making changes to the environment via setenv, I think we need to base it off the environ pointer after setenv has been called rather than the envp from main.

That said, I don't think the envp variable we are storing is being used for anything at the moment (reading environment variables was changed to use getenv rather than envp in the commit here). If removing the usage of environ is preferable, we could maybe remove the usage of envp altogether (in a separate patch)--does this sound like a good idea or would it be better to just leave in the environ usage for now?

flang/test/Driver/emit-mlir.f90
16

Agreed that there are potential risks with the current approach (although, are the _Q* names considered reserved?). Unfortunately, I think generating a call to set the environment defaults requires somewhat significant changes to the runtime. The runtime reads environment variables during initialization in ExecutionEnvironment::Configure which is ultimately called from the "hardcoded" Fortran_main.c and I need to set the defaults before this happens. So, I believe I'd either have to move the initialization to _QQmain or make it so that main isn't hardcoded so that I could insert the appropriate runtime function.

@klausler I think I asked you about this when I was first trying to figure out how to implement the environment defaults and you suggested I try the extern approach--please let me know if you have thoughts/suggestions around this!

peixin added inline comments.Sep 16 2022, 6:53 PM
flang/runtime/environment.cpp
77

Thanks for the explanations. The current fix makes sense to me.

flang/test/Driver/emit-mlir.f90
16

This is what @klausler suggested:

Instead of adding new custom APIs that let command-line options control behavior in a way that is redundant with the runtime environment, I suggest that you try a more general runtime library API by which the main program can specify a default environment variable setting, or a set of them. Then turn the command-line options into the equivalent environment settings and pass them as default settings that could be overridden by the actual environment.

If I understand correctly, what I am suggesting match his comments. The "main program" he means should be fortran main program, not the RTNAME(ProgramStart. In your initial patch, you add the runtime specified for "convert option". I think @klausler suggest you making the runtime argument more general used for a set of runtime environment variable settings, not restricted to "convert option". And that is what you already added -- EnvironmentDefaultList. So, combining this patch and your initial patch will be the solution. Hope I understand it correctly.

jpenix-quic added inline comments.Sep 19 2022, 7:20 PM
flang/test/Driver/emit-mlir.f90
16

The issue I hit with the suggested approach is that in order to use the pre-existing runtime environment variable handling to set the internal state I need to set the environment variable defaults before the environment variables are read by the runtime.

I might be misunderstanding/missing something, but given that the environment variables are read as part of RTNAME(ProgramStart) in main and the earliest I can place the call if I am generating it is _QQmain, I think that leaves three options: 1. don't hardcode main so that I can place the call early enough 2. delay or rerun the code here that is responsible for initializing the runtime state so that it is called as part of _QQmain so I can insert my runtime function before it or 3. hardcode something like the _QQEnvironmentDefaults into Fortran_main.c so that the environment defaults are available early enough. Option 2 seems less than ideal to me, option 1 seems ideal but requires generating main, so option 3 is what I ended up going with. If options 1 or 2 would be preferable to what is currently implemented (or if there is another possibility I'm missing!) I'd be happy to switch and try to implement them.

peixin added a reviewer: jeanPerier.
peixin added inline comments.Sep 20 2022, 12:24 AM
flang/test/Driver/emit-mlir.f90
16

What do other reviewers think? @klausler @awarzynski @kiranchandramohan @clementval @jeanPerier
The current approach has two drawbacks:

  1. Add one extern variable _QQEnvironmentDefaults in runtime.
  2. Generate the variable during lowering even if there is no -fconvert option.

Can we accept this?

awarzynski added inline comments.Sep 20 2022, 3:20 AM
flang/include/flang/Optimizer/Builder/Runtime/EnvironmentDefaults.h
8

Could you document what these are? And what are they used for?

flang/lib/Frontend/CompilerInvocation.cpp
414–424

I'm OK with a lambda here, just pointing our that in other cases we added small hooks, e.g. getOptimizationLevel. I personally prefer hooks as this means that methods like parseFrontendArgs can be a bit shorter. But this is a very weak preference!

flang/test/Driver/convert.f90
2

Could you be more specific? IIUC, this is more about making sure that the option parser works correctly and reports invalid usage of -fconvert as an error, right?

flang/test/Driver/emit-mlir.f90
16

Based on the analysis by @jpenix-quic, this is the path of least resistance and SGTM. It can always be refactored/improved in the future if need be. It would be helpful if this was documented somewhere. But I'm not sure whether there's a good landing space for this ATM.

flang/test/Lower/environment-defaults.f90
2

Can you test with flang-new as well?

rovka added a subscriber: rovka.Sep 20 2022, 4:50 AM

IMO this is a reasonable approach, I only have a few nits.

flang/runtime/environment.cpp
42
77

I agree that we should remove envp altogether in a follow-up patch. As it stands it's just causing confusion.

flang/test/Driver/convert.f90
2

Nit: Shouldn't you also check that valid values (e.g. unknown, swap etc) are accepted?

flang/test/Driver/driver-help.f90
27

Nit: Why is there an extra blank for these?

peixin added inline comments.Sep 20 2022, 5:47 AM
flang/test/Driver/emit-mlir.f90
16

How about adding FIXME when generating the variables during lowering (flang/lib/Lower/Bridge.cpp).

Address comments from @awarzynski, @rovka, and @peixin. Also fixed the file header comments for EnvironmentDefaults.h, environment-defaults.h, and environment-default-list.h to match others in their respective folders.

jpenix-quic marked an inline comment as done.

Move comment, see if build will cooperate.

jpenix-quic marked 5 inline comments as done.Sep 20 2022, 5:16 PM
jpenix-quic added inline comments.
flang/test/Driver/convert.f90
2

Nit: Shouldn't you also check that valid values (e.g. unknown, swap etc) are accepted?

I might be doing something silly/missing something obvious, but I wasn't sure how to get FileCheck to basically just check that the command didn't error without adding checks, so I added trivial checks (VALID/VALID_FC1) for each of unknown/native/etc. Is there a better way of doing this? I also expanded flang/test/Lower/convert.f90 to check lowering for each of the valid options, so I didn't want to do full checks of the MLIR here.

The build still fails.

The build still fails.

I think it might be an infrastructure issue or something like that--I've tried restarting it a few times and each time it just ends with "HTTP 28" as the only message I see. Another review that was created a bit ago (https://reviews.llvm.org/D134329) seems to have the same issue, so I'll try restarting it again in a while.

The build still fails.

I think it might be an infrastructure issue or something like that--I've tried restarting it a few times and each time it just ends with "HTTP 28" as the only message I see. Another review that was created a bit ago (https://reviews.llvm.org/D134329) seems to have the same issue, so I'll try restarting it again in a while.

All the new created reviews fail for the same reason. https://reviews.llvm.org/harbormaster. You may need to wait for some time to restart it.

LGTM. Tested on one little-endian machine with our internal tests (I attached them in the issue https://github.com/llvm/llvm-project/issues/55961) for the priority of -fconvert= option and CONVERT argument in open statement and all passed. I didn't see the the GFORTRAN_CONVERT_UNIT used in HPC workloads and don't have tests for it. For now, I would avoid full combination tests for it.

@jeanPerier @clementval Can you help double check the lowering part?

rovka added inline comments.Sep 29 2022, 1:25 AM
flang/test/Driver/convert.f90
2

Just in case it's not clear, this looks good to me now, it's exactly what I had in mind.

jeanPerier accepted this revision.Sep 30 2022, 7:05 AM

The lowering part looks good to me (I only have a minor comment inlined about a header used in lowering).

flang/include/flang/Runtime/environment-defaults.h
19 ↗(On Diff #461759)

I think this header may better belong to Semantics (or even Lower since it is only used there) in the sense that it does not define a data structure that is used at runtime, but it defines a data structure so that we can keep track of some default environment value during compilation (It is not a huge deal, but I am a little bit wary of seeing std::string in Fortran::runtime while the runtime is meant to be free of the C++ runtime).

This revision is now accepted and ready to land.Sep 30 2022, 7:05 AM

Add /*overwrite=*/ comment I missed previously, move Runtime/environment-defaults.h to Lower/EnvironmentDefault.h

jpenix-quic marked an inline comment as done.Oct 3 2022, 4:48 PM

The lowering part looks good to me (I only have a minor comment inlined about a header used in lowering).

Thank you @jeanPerier for looking over the lowering portion! Regarding moving the header (I'm replying to the comment here since the inline one now opens in a separate revision/window as the file is gone), I moved it to Lower/EnvironmentDefault.h as lowering is where I use it (as you mentioned). Please let me know if this needs further changes!

Just to confirm/as a final check, @awarzynski does the driver portion look good to you? Similarly, @klausler does the runtime portion look good to you? I believe I addressed the comments you both left previously, but I want to make sure that I addressed things appropriately. Thanks!

jeanPerier accepted this revision.Oct 4 2022, 1:52 AM

Thank you @jeanPerier for looking over the lowering portion! Regarding moving the header (I'm replying to the comment here since the inline one now opens in a separate revision/window as the file is gone), I moved it to Lower/EnvironmentDefault.h as lowering is where I use it (as you mentioned). Please let me know if this needs further changes!

LGTM, thanks.

jpenix-quic added a comment.EditedOct 12 2022, 11:55 AM

Ping @awarzynski and @klausler --could you please let me know if the driver and runtime portions look good to you, respectively? (Apologies in advance if this isn't necessary, but as you both gave me significant feedback I wanted to make sure you are ok with the patch as it stands.) Thanks!

klausler accepted this revision.Oct 12 2022, 11:59 AM

The runtime parts look good to me.

awarzynski accepted this revision.Oct 12 2022, 2:01 PM

The driver looks good, thanks for all the effort!

jpenix-quic closed this revision.Oct 12 2022, 5:25 PM

0ec3ac9b7fbd

Gah, I goofed and forgot to add the "Differential Revision" line--this has been committed at 0ec3ac9b7fbd15698af7289e1214e8ff3d82ec14.