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

jpenix-quic closed this revision. jpenix-quic added a comment. 0ec3ac9b7fbd Gah, I goofed and forgot to add the "Differential Revision" line--this has been committed at 0ec3ac9b7fbd15698af7289e1214e8ff3d82ec14

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

awarzynski accepted this revision. awarzynski added a comment. The driver looks good, thanks for all the effort! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130513/new/ https://reviews.llvm.org/D130513 ___ cfe-commits mailing list

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

klausler accepted this revision. klausler added a comment. The runtime parts look good to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130513/new/ https://reviews.llvm.org/D130513 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

jpenix-quic added a comment. 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] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

jeanPerier accepted this revision. jeanPerier added a comment. In D130513#3832241 , @jpenix-quic wrote: > Thank you @jeanPerier for looking over the lowering portion! Regarding moving > the header (I'm replying to the comment here since the inline one

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

jpenix-quic marked an inline comment as done. jpenix-quic added a comment. In D130513#3827108 , @jeanPerier wrote: > The lowering part looks good to me (I only have a minor comment inlined about > a header used in lowering). Thank you @jeanPerier for

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

jpenix-quic updated this revision to Diff 464849. jpenix-quic added a comment. Add /*overwrite=*/ comment I missed previously, move Runtime/environment-defaults.h to Lower/EnvironmentDefault.h CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130513/new/ https://reviews.llvm.org/D130513

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

jeanPerier accepted this revision. jeanPerier added a comment. This revision is now accepted and ready to land. The lowering part looks good to me (I only have a minor comment inlined about a header used in lowering). Comment at:

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

rovka added inline comments. Herald added a subscriber: zero9178. Comment at: flang/test/Driver/convert.f90:1 +! Ensure argument -fconvert= works as expected. + jpenix-quic wrote: > awarzynski wrote: > > rovka wrote: > > > Nit: Shouldn't you also check that

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

peixin added a comment. 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

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

peixin added a comment. In D130513#3804659 , @jpenix-quic wrote: >> 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 >

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

jpenix-quic added a comment. > 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

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

peixin added a comment. The build still fails. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130513/new/ https://reviews.llvm.org/D130513 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

jpenix-quic marked 5 inline comments as done. jpenix-quic added inline comments. Comment at: flang/test/Driver/convert.f90:1 +! Ensure argument -fconvert= works as expected. + awarzynski wrote: > rovka wrote: > > Nit: Shouldn't you also check that valid values

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

jpenix-quic updated this revision to Diff 461759. jpenix-quic marked an inline comment as done. jpenix-quic added a comment. Move comment, see if build will cooperate. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130513/new/ https://reviews.llvm.org/D130513 Files:

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

jpenix-quic updated this revision to Diff 461753. jpenix-quic added a comment. 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

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

peixin added inline comments. Comment at: flang/test/Driver/emit-mlir.f90:16 ! CHECK-NEXT: } +! CHECK-NEXT: fir.global @_QQEnvironmentDefaults constant : !fir.ref, !fir.ref> { +! CHECK-NEXT: %[[VAL_0:.*]] = fir.zero_bits !fir.ref, !fir.ref>

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

rovka added a comment. IMO this is a reasonable approach, I only have a few nits. Comment at: flang/runtime/environment.cpp:42 +#else +if (setenv(name, value, 0) == -1) { +#endif Comment at: flang/runtime/environment.cpp:77 +#else +

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

awarzynski added inline comments. Comment at: flang/include/flang/Optimizer/Builder/Runtime/EnvironmentDefaults.h:7 +// +//===--===// + Could you document what these are? And what are they used

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

peixin added inline comments. Comment at: flang/test/Driver/emit-mlir.f90:16 ! CHECK-NEXT: } +! CHECK-NEXT: fir.global @_QQEnvironmentDefaults constant : !fir.ref, !fir.ref> { +! CHECK-NEXT: %[[VAL_0:.*]] = fir.zero_bits !fir.ref, !fir.ref>

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

jpenix-quic added inline comments. Comment at: flang/test/Driver/emit-mlir.f90:16 ! CHECK-NEXT: } +! CHECK-NEXT: fir.global @_QQEnvironmentDefaults constant : !fir.ref, !fir.ref> { +! CHECK-NEXT: %[[VAL_0:.*]] = fir.zero_bits !fir.ref, !fir.ref>

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

peixin added inline comments. Comment at: flang/runtime/environment.cpp:77 +#else + envp = environ; +#endif jpenix-quic wrote: > peixin wrote: > > What is `environ` used for? Should we try to keep as less extern variable > > as possible in runtime for

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

jpenix-quic marked 2 inline comments as done. jpenix-quic added inline comments. Comment at: flang/runtime/environment.cpp:77 +#else + envp = environ; +#endif peixin wrote: > What is `environ` used for? Should we try to keep as less extern variable as >

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

jpenix-quic updated this revision to Diff 460819. jpenix-quic added a comment. Remove unneeded braces and unnecessary changes to NameUniquer. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130513/new/ https://reviews.llvm.org/D130513 Files: clang/include/clang/Driver/Options.td

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

peixin added inline comments. Comment at: flang/runtime/environment.cpp:77 +#else + envp = environ; +#endif What is `environ` used for? Should we try to keep as less extern variable as possible in runtime for security. Comment at:

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

jpenix-quic added inline comments. Comment at: flang/test/Driver/emit-mlir.f90:16 ! CHECK-NEXT: } +! CHECK-NEXT: fir.global @_QQEnvironmentDefaults constant : !fir.ref, !fir.ref> { +! CHECK-NEXT: %[[VAL_0:.*]] = fir.zero_bits !fir.ref, !fir.ref>

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

peixin added a comment. Mostly LGTM. Have several more comments. Comment at: flang/lib/Lower/Bridge.cpp:279 +// are compiled separately. +if (hasMainProgram) { + createGlobalOutsideOfFunctionLowering([&]() { Nit Comment at:

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

peixin added a comment. $ 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:

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

jpenix-quic added inline comments. Comment at: flang/runtime/environment-default-list.h:1 +//===-- Runtime/environment-default-list.h ===// +// klausler wrote: > If you want this header to be maximally portable C and C++, please

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

jpenix-quic updated this revision to Diff 455975. jpenix-quic added a comment. Fixed up/simplified environment-default-list.h for C/C++ compatibility. Also cleaned up a declaration and a few autos in the lowering component. Rebased. CHANGES SINCE LAST ACTION

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

peixin added a comment. This needs rebase. I failed to apply this patch due to conflict. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130513/new/ https://reviews.llvm.org/D130513 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

klausler added a comment. I can't speak to the lowering or driver bits, but the runtime part looks pretty good to me. Comment at: flang/runtime/environment-default-list.h:1 +//===-- Runtime/environment-default-list.h ===// +//

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

jpenix-quic added a comment. @klausler Could you please take a look at this again and let me know if it is more in line with your suggestion above? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130513/new/ https://reviews.llvm.org/D130513

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

jpenix-quic updated this revision to Diff 451225. jpenix-quic added a comment. Rebase to address conflicts, use string substitution blocks in my tests. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130513/new/ https://reviews.llvm.org/D130513 Files:

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

peixin added a comment. This needs rebase. Comment at: flang/lib/Lower/Bridge.cpp:203 //them. for (Fortran::lower::pft::Program::Units : pft.getUnits()) { std::visit(Fortran::common::visitors{ jpenix-quic wrote: > peixin wrote: > > Doing

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

jpenix-quic added inline comments. Comment at: flang/lib/Lower/Bridge.cpp:203 //them. for (Fortran::lower::pft::Program::Units : pft.getUnits()) { std::visit(Fortran::common::visitors{ peixin wrote: > Doing this can avoid add one variable

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

jpenix-quic updated this revision to Diff 450895. jpenix-quic edited the summary of this revision. jpenix-quic added a comment. Change hasMainProgram to be a local variable and update summary to reflect the new approach. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130513/new/

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

peixin added inline comments. Comment at: flang/lib/Lower/Bridge.cpp:203 //them. for (Fortran::lower::pft::Program::Units : pft.getUnits()) { std::visit(Fortran::common::visitors{ Doing this can avoid add one variable to the bridge.

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

jpenix-quic updated this revision to Diff 449894. jpenix-quic added a comment. Hopefully fix Window's build errors by using the proper Windows-specific environment utilities (_putenv_s vs setenv). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130513/new/

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

jpenix-quic added a comment. > BTW, can you continue working on the lowering of the convert argument of open > statement? I will take a look at this! Comment at: flang/lib/Lower/Bridge.cpp:2757 +if (funit.isMainProgram()) { + auto conversion =

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

jpenix-quic updated this revision to Diff 449875. jpenix-quic added a comment. Herald added subscribers: bzcheeseman, sdasgup3, wenzhicui, wrengr, cota, teijeong, rdzhabarov, tatianashp, msifontes, jurahul, Kayjukh, grosul1, Joonsoo, stephenneuendorffer, liufengdb, aartbik, mgester,

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

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

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

jpenix-quic marked 2 inline comments as done. jpenix-quic added inline comments. Comment at: clang/lib/Driver/ToolChains/Flang.cpp:52 +options::OPT_fno_automatic, +options::OPT_fconvert_EQ}); } awarzynski

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

jpenix-quic updated this revision to Diff 447838. jpenix-quic added a comment. 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. CHANGES

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

awarzynski added a comment. Thanks for working on this. This is not my area of expertise, so I focused on the implementation in the driver. Comment at: clang/include/clang/Driver/Options.td:4897 def ffixed_line_length_VALUE : Joined<["-"], "ffixed-line-length-">, Group,

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

jpenix-quic added inline comments. Comment at: clang/include/clang/Driver/Options.td:4897 def ffixed_line_length_VALUE : Joined<["-"], "ffixed-line-length-">, Group, Alias; +def fconvert_EQ : Joined<["-"], "fconvert=">, Group, + HelpText<"Set endian conversion of data for

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

peixin added inline comments. Comment at: clang/include/clang/Driver/Options.td:4897 def ffixed_line_length_VALUE : Joined<["-"], "ffixed-line-length-">, Group, Alias; +def fconvert_EQ : Joined<["-"], "fconvert=">, Group, + HelpText<"Set endian conversion of data for

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

jpenix-quic added a comment. 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

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

klausler added inline comments. Comment at: flang/runtime/main.cpp:51 + if (Fortran::runtime::executionEnvironment.conversion != + Fortran::runtime::Convert::Unknown) +return; We always use braces on if/for/while/ constructs in the runtime.

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

klausler added a comment. Instead of adding new custom APIs that let command-line options control behavior is 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

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

jpenix-quic added a comment. 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).

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

jpenix-quic created this revision. jpenix-quic added reviewers: schweitz, klausler, peixin, awarzynski. jpenix-quic added a project: Flang. Herald added subscribers: mehdi_amini, jdoerfert, mgorny. Herald added a reviewer: sscalpone. Herald added a project: All. jpenix-quic requested review of