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

[Jonathon Penix via Phabricator via cfe-commits]

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 
.


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Andrzej Warzynski via Phabricator via cfe-commits]

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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Peter Klausler via Phabricator via cfe-commits]

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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonathon Penix via Phabricator via cfe-commits]

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 as it stands.) Thanks!


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jean Perier via Phabricator via cfe-commits]

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 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.


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonathon Penix via Phabricator via cfe-commits]

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 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!


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonathon Penix via Phabricator via cfe-commits]

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

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/examples/external-hello.cpp
  flang/include/flang/Frontend/FrontendOptions.h
  flang/include/flang/Lower/Bridge.h
  flang/include/flang/Lower/EnvironmentDefault.h
  flang/include/flang/Optimizer/Builder/Runtime/EnvironmentDefaults.h
  flang/include/flang/Runtime/main.h
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/lib/Lower/Bridge.cpp
  flang/lib/Optimizer/Builder/CMakeLists.txt
  flang/lib/Optimizer/Builder/Runtime/EnvironmentDefaults.cpp
  flang/runtime/FortranMain/Fortran_main.c
  flang/runtime/environment-default-list.h
  flang/runtime/environment.cpp
  flang/runtime/environment.h
  flang/runtime/main.cpp
  flang/test/Driver/convert.f90
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-help.f90
  flang/test/Driver/emit-mlir.f90
  flang/test/Driver/frontend-forwarding.f90
  flang/test/Lower/convert.f90
  flang/test/Lower/environment-defaults.f90
  flang/test/Runtime/no-cpp-dep.c
  flang/tools/bbc/bbc.cpp
  flang/unittests/Runtime/CommandTest.cpp
  flang/unittests/Runtime/Stop.cpp

Index: flang/unittests/Runtime/Stop.cpp
===
--- flang/unittests/Runtime/Stop.cpp
+++ flang/unittests/Runtime/Stop.cpp
@@ -26,7 +26,8 @@
 
 TEST(TestProgramEnd, StopTestNoStopMessage) {
   putenv(const_cast("NO_STOP_MESSAGE=1"));
-  Fortran::runtime::executionEnvironment.Configure(0, nullptr, nullptr);
+  Fortran::runtime::executionEnvironment.Configure(
+  0, nullptr, nullptr, nullptr);
   EXPECT_EXIT(
   RTNAME(StopStatement)(), testing::ExitedWithCode(EXIT_SUCCESS), "");
 }
@@ -52,7 +53,8 @@
 
 TEST(TestProgramEnd, NoStopMessageTest) {
   putenv(const_cast("NO_STOP_MESSAGE=1"));
-  Fortran::runtime::executionEnvironment.Configure(0, nullptr, nullptr);
+  Fortran::runtime::executionEnvironment.Configure(
+  0, nullptr, nullptr, nullptr);
   static const char *message{"bye bye"};
   EXPECT_EXIT(RTNAME(StopStatementText)(message, std::strlen(message),
   /*isErrorStop=*/false, /*quiet=*/false),
Index: flang/unittests/Runtime/CommandTest.cpp
===
--- flang/unittests/Runtime/CommandTest.cpp
+++ flang/unittests/Runtime/CommandTest.cpp
@@ -49,7 +49,7 @@
 class CommandFixture : public ::testing::Test {
 protected:
   CommandFixture(int argc, const char *argv[]) {
-RTNAME(ProgramStart)(argc, argv, {});
+RTNAME(ProgramStart)(argc, argv, {}, {});
   }
 
   std::string GetPaddedStr(const char *text, std::size_t len) const {
Index: flang/tools/bbc/bbc.cpp
===
--- flang/tools/bbc/bbc.cpp
+++ flang/tools/bbc/bbc.cpp
@@ -224,7 +224,7 @@
   auto burnside = Fortran::lower::LoweringBridge::create(
   ctx, semanticsContext, defKinds, semanticsContext.intrinsics(),
   semanticsContext.targetCharacteristics(), parsing.allCooked(), "",
-  kindMap, loweringOptions);
+  kindMap, loweringOptions, {});
   burnside.lower(parseTree, semanticsContext);
   mlir::ModuleOp mlirModule = burnside.getModule();
   std::error_code ec;
Index: flang/test/Runtime/no-cpp-dep.c
===
--- flang/test/Runtime/no-cpp-dep.c
+++ flang/test/Runtime/no-cpp-dep.c
@@ -16,18 +16,20 @@
 we're testing. We can't include any headers directly since they likely contain
 C++ code that would explode here.
 */
+struct EnvironmentDefaultList;
 struct Descriptor;
 
 double RTNAME(CpuTime)();
 
-void RTNAME(ProgramStart)(int, const char *[], const char *[]);
+void RTNAME(ProgramStart)(
+int, const char *[], const char *[], const struct EnvironmentDefaultList *);
 int32_t RTNAME(ArgumentCount)();
 int32_t RTNAME(GetCommandArgument)(int32_t, const struct Descriptor *,
 const struct Descriptor *, const struct Descriptor *);
 
 int main() {
   double x = RTNAME(CpuTime)();
-  RTNAME(ProgramStart)(0, 0, 0);
+  RTNAME(ProgramStart)(0, 0, 0, 0);
   int32_t c = RTNAME(ArgumentCount)();
   int32_t v = RTNAME(GetCommandArgument)(0, 0, 0, 0);
   return x + c + v;
Index: flang/test/Lower/environment-defaults.f90
===
--- /dev/null
+++ flang/test/Lower/environment-defaults.f90
@@ -0,0 +1,12 @@
+! RUN: %flang_fc1 -emit-fir %s -o - | FileCheck %s
+! RUN: bbc -emit-fir -o - %s | FileCheck %s
+
+program test
+  continue
+end
+
+! Test that a null pointer is generated for environment defaults if nothing is specified
+
+! CHECK: fir.global @_QQEnvironmentDefaults constant : !fir.ref, 

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

[Jean Perier via Phabricator via cfe-commits]

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: flang/include/flang/Runtime/environment-defaults.h:19
+  std::string defaultValue;
+};
+

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).


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Diana Picus via Phabricator via cfe-commits]

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 valid values (e.g. unknown, swap etc) 
> > > are accepted? 
> > 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?
> > 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. 
Just in case it's not clear, this looks good to me now, it's exactly what I had 
in mind.


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Peixin Qiao via Phabricator via cfe-commits]

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 `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?


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Peixin Qiao via Phabricator via cfe-commits]

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 
> 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.


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonathon Penix via Phabricator via cfe-commits]

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 
(https://reviews.llvm.org/D134329) seems to have the same issue, so I'll try 
restarting it again in a while.


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Peixin Qiao via Phabricator via cfe-commits]

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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonathon Penix via Phabricator via cfe-commits]

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 (e.g. unknown, swap etc) 
> > are accepted? 
> 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?
> 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. 


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonathon Penix via Phabricator via cfe-commits]

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:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/examples/external-hello.cpp
  flang/include/flang/Frontend/FrontendOptions.h
  flang/include/flang/Lower/Bridge.h
  flang/include/flang/Optimizer/Builder/Runtime/EnvironmentDefaults.h
  flang/include/flang/Runtime/environment-defaults.h
  flang/include/flang/Runtime/main.h
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/lib/Lower/Bridge.cpp
  flang/lib/Optimizer/Builder/CMakeLists.txt
  flang/lib/Optimizer/Builder/Runtime/EnvironmentDefaults.cpp
  flang/runtime/FortranMain/Fortran_main.c
  flang/runtime/environment-default-list.h
  flang/runtime/environment.cpp
  flang/runtime/environment.h
  flang/runtime/main.cpp
  flang/test/Driver/convert.f90
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-help.f90
  flang/test/Driver/emit-mlir.f90
  flang/test/Driver/frontend-forwarding.f90
  flang/test/Lower/convert.f90
  flang/test/Lower/environment-defaults.f90
  flang/test/Runtime/no-cpp-dep.c
  flang/tools/bbc/bbc.cpp
  flang/unittests/Runtime/CommandTest.cpp
  flang/unittests/Runtime/Stop.cpp

Index: flang/unittests/Runtime/Stop.cpp
===
--- flang/unittests/Runtime/Stop.cpp
+++ flang/unittests/Runtime/Stop.cpp
@@ -26,7 +26,8 @@
 
 TEST(TestProgramEnd, StopTestNoStopMessage) {
   putenv(const_cast("NO_STOP_MESSAGE=1"));
-  Fortran::runtime::executionEnvironment.Configure(0, nullptr, nullptr);
+  Fortran::runtime::executionEnvironment.Configure(
+  0, nullptr, nullptr, nullptr);
   EXPECT_EXIT(
   RTNAME(StopStatement)(), testing::ExitedWithCode(EXIT_SUCCESS), "");
 }
@@ -52,7 +53,8 @@
 
 TEST(TestProgramEnd, NoStopMessageTest) {
   putenv(const_cast("NO_STOP_MESSAGE=1"));
-  Fortran::runtime::executionEnvironment.Configure(0, nullptr, nullptr);
+  Fortran::runtime::executionEnvironment.Configure(
+  0, nullptr, nullptr, nullptr);
   static const char *message{"bye bye"};
   EXPECT_EXIT(RTNAME(StopStatementText)(message, std::strlen(message),
   /*isErrorStop=*/false, /*quiet=*/false),
Index: flang/unittests/Runtime/CommandTest.cpp
===
--- flang/unittests/Runtime/CommandTest.cpp
+++ flang/unittests/Runtime/CommandTest.cpp
@@ -49,7 +49,7 @@
 class CommandFixture : public ::testing::Test {
 protected:
   CommandFixture(int argc, const char *argv[]) {
-RTNAME(ProgramStart)(argc, argv, {});
+RTNAME(ProgramStart)(argc, argv, {}, {});
   }
 
   std::string GetPaddedStr(const char *text, std::size_t len) const {
Index: flang/tools/bbc/bbc.cpp
===
--- flang/tools/bbc/bbc.cpp
+++ flang/tools/bbc/bbc.cpp
@@ -222,7 +222,7 @@
   auto burnside = Fortran::lower::LoweringBridge::create(
   ctx, semanticsContext, defKinds, semanticsContext.intrinsics(),
   semanticsContext.targetCharacteristics(), parsing.allCooked(), "",
-  kindMap, loweringOptions);
+  kindMap, loweringOptions, {});
   burnside.lower(parseTree, semanticsContext);
   mlir::ModuleOp mlirModule = burnside.getModule();
   std::error_code ec;
Index: flang/test/Runtime/no-cpp-dep.c
===
--- flang/test/Runtime/no-cpp-dep.c
+++ flang/test/Runtime/no-cpp-dep.c
@@ -16,18 +16,20 @@
 we're testing. We can't include any headers directly since they likely contain
 C++ code that would explode here.
 */
+struct EnvironmentDefaultList;
 struct Descriptor;
 
 double RTNAME(CpuTime)();
 
-void RTNAME(ProgramStart)(int, const char *[], const char *[]);
+void RTNAME(ProgramStart)(
+int, const char *[], const char *[], const struct EnvironmentDefaultList *);
 int32_t RTNAME(ArgumentCount)();
 int32_t RTNAME(GetCommandArgument)(int32_t, const struct Descriptor *,
 const struct Descriptor *, const struct Descriptor *);
 
 int main() {
   double x = RTNAME(CpuTime)();
-  RTNAME(ProgramStart)(0, 0, 0);
+  RTNAME(ProgramStart)(0, 0, 0, 0);
   int32_t c = RTNAME(ArgumentCount)();
   int32_t v = RTNAME(GetCommandArgument)(0, 0, 0, 0);
   return x + c + v;
Index: flang/test/Lower/environment-defaults.f90
===
--- /dev/null
+++ flang/test/Lower/environment-defaults.f90
@@ -0,0 +1,12 @@
+! RUN: %flang_fc1 -emit-fir %s -o - | FileCheck %s
+! RUN: bbc -emit-fir -o - %s | FileCheck %s
+
+program test
+  continue
+end
+
+! Test that a null pointer is generated for environment defaults if nothing is specified
+
+! CHECK: fir.global @_QQEnvironmentDefaults constant : !fir.ref, !fir.ref> {
+! CHECK: 

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

[Jonathon Penix via Phabricator via cfe-commits]

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 folders.


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

https://reviews.llvm.org/D130513

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/examples/external-hello.cpp
  flang/include/flang/Frontend/FrontendOptions.h
  flang/include/flang/Lower/Bridge.h
  flang/include/flang/Optimizer/Builder/Runtime/EnvironmentDefaults.h
  flang/include/flang/Runtime/environment-defaults.h
  flang/include/flang/Runtime/main.h
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/lib/Lower/Bridge.cpp
  flang/lib/Optimizer/Builder/CMakeLists.txt
  flang/lib/Optimizer/Builder/Runtime/EnvironmentDefaults.cpp
  flang/runtime/FortranMain/Fortran_main.c
  flang/runtime/environment-default-list.h
  flang/runtime/environment.cpp
  flang/runtime/environment.h
  flang/runtime/main.cpp
  flang/test/Driver/convert.f90
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-help.f90
  flang/test/Driver/emit-mlir.f90
  flang/test/Driver/frontend-forwarding.f90
  flang/test/Lower/convert.f90
  flang/test/Lower/environment-defaults.f90
  flang/test/Runtime/no-cpp-dep.c
  flang/tools/bbc/bbc.cpp
  flang/unittests/Runtime/CommandTest.cpp
  flang/unittests/Runtime/Stop.cpp

Index: flang/unittests/Runtime/Stop.cpp
===
--- flang/unittests/Runtime/Stop.cpp
+++ flang/unittests/Runtime/Stop.cpp
@@ -26,7 +26,8 @@
 
 TEST(TestProgramEnd, StopTestNoStopMessage) {
   putenv(const_cast("NO_STOP_MESSAGE=1"));
-  Fortran::runtime::executionEnvironment.Configure(0, nullptr, nullptr);
+  Fortran::runtime::executionEnvironment.Configure(
+  0, nullptr, nullptr, nullptr);
   EXPECT_EXIT(
   RTNAME(StopStatement)(), testing::ExitedWithCode(EXIT_SUCCESS), "");
 }
@@ -52,7 +53,8 @@
 
 TEST(TestProgramEnd, NoStopMessageTest) {
   putenv(const_cast("NO_STOP_MESSAGE=1"));
-  Fortran::runtime::executionEnvironment.Configure(0, nullptr, nullptr);
+  Fortran::runtime::executionEnvironment.Configure(
+  0, nullptr, nullptr, nullptr);
   static const char *message{"bye bye"};
   EXPECT_EXIT(RTNAME(StopStatementText)(message, std::strlen(message),
   /*isErrorStop=*/false, /*quiet=*/false),
Index: flang/unittests/Runtime/CommandTest.cpp
===
--- flang/unittests/Runtime/CommandTest.cpp
+++ flang/unittests/Runtime/CommandTest.cpp
@@ -49,7 +49,7 @@
 class CommandFixture : public ::testing::Test {
 protected:
   CommandFixture(int argc, const char *argv[]) {
-RTNAME(ProgramStart)(argc, argv, {});
+RTNAME(ProgramStart)(argc, argv, {}, {});
   }
 
   std::string GetPaddedStr(const char *text, std::size_t len) const {
Index: flang/tools/bbc/bbc.cpp
===
--- flang/tools/bbc/bbc.cpp
+++ flang/tools/bbc/bbc.cpp
@@ -222,7 +222,7 @@
   auto burnside = Fortran::lower::LoweringBridge::create(
   ctx, semanticsContext, defKinds, semanticsContext.intrinsics(),
   semanticsContext.targetCharacteristics(), parsing.allCooked(), "",
-  kindMap, loweringOptions);
+  kindMap, loweringOptions, {});
   burnside.lower(parseTree, semanticsContext);
   mlir::ModuleOp mlirModule = burnside.getModule();
   std::error_code ec;
Index: flang/test/Runtime/no-cpp-dep.c
===
--- flang/test/Runtime/no-cpp-dep.c
+++ flang/test/Runtime/no-cpp-dep.c
@@ -16,18 +16,20 @@
 we're testing. We can't include any headers directly since they likely contain
 C++ code that would explode here.
 */
+struct EnvironmentDefaultList;
 struct Descriptor;
 
 double RTNAME(CpuTime)();
 
-void RTNAME(ProgramStart)(int, const char *[], const char *[]);
+void RTNAME(ProgramStart)(
+int, const char *[], const char *[], const struct EnvironmentDefaultList *);
 int32_t RTNAME(ArgumentCount)();
 int32_t RTNAME(GetCommandArgument)(int32_t, const struct Descriptor *,
 const struct Descriptor *, const struct Descriptor *);
 
 int main() {
   double x = RTNAME(CpuTime)();
-  RTNAME(ProgramStart)(0, 0, 0);
+  RTNAME(ProgramStart)(0, 0, 0, 0);
   int32_t c = RTNAME(ArgumentCount)();
   int32_t v = RTNAME(GetCommandArgument)(0, 0, 0, 0);
   return x + c + v;
Index: flang/test/Lower/environment-defaults.f90
===
--- /dev/null
+++ flang/test/Lower/environment-defaults.f90
@@ -0,0 +1,12 @@
+! RUN: %flang_fc1 -emit-fir %s -o - | FileCheck %s
+! RUN: bbc -emit-fir -o - %s | FileCheck %s
+
+program test
+  continue
+end
+
+! Test that a null pointer is generated for 

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

[Peixin Qiao via Phabricator via cfe-commits]

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>

awarzynski wrote:
> peixin wrote:
> > jpenix-quic wrote:
> > > peixin wrote:
> > > > jpenix-quic wrote:
> > > > > peixin wrote:
> > > > > > jpenix-quic wrote:
> > > > > > > peixin wrote:
> > > > > > > > Is it possible not to generated this global variable if 
> > > > > > > > `fconvert=` is not specified?
> > > > > > > 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?)
> > > > > > 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?
> > > > > 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!
> > > > 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.
> > > 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 

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

[Diana Picus via Phabricator via cfe-commits]

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
+  envp = environ;
+#endif

peixin wrote:
> jpenix-quic wrote:
> > peixin wrote:
> > > What is `environ` used for? Should we try to keep as less extern variable 
> > > as possible in runtime for security.
> > 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 [[ 
> > https://github.com/llvm/llvm-project/commit/824bf908194c9267f1f09065ba8e41d7969006ab
> >  | 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?
> Thanks for the explanations. The current fix makes sense to me.
I agree that we should remove envp altogether in a follow-up patch. As it 
stands it's just causing confusion.



Comment at: flang/test/Driver/convert.f90:1
+! Ensure argument -fconvert= works as expected.
+

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



Comment at: flang/test/Driver/driver-help.f90:27
 ! HELP-NEXT: -fcolor-diagnosticsEnable colors in diagnostics
+! HELP-NEXT: -fconvert=   Set endian conversion of data for 
unformatted files
 ! HELP-NEXT: -fdefault-double-8 Set the default double precision kind to 
an 8 byte wide type

Nit: Why is there an extra blank for these?


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Andrzej Warzynski via Phabricator via cfe-commits]

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 for?



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:403-413
+  if (const auto *arg =
+  args.getLastArg(clang::driver::options::OPT_fconvert_EQ)) {
+auto parseConvertArg = [](const char *s) {
+  return llvm::StringSwitch>(s)
+  .Case("unknown", "UNKNOWN")
+  .Case("native", "NATIVE")
+  .Case("little-endian", "LITTLE_ENDIAN")

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!



Comment at: flang/test/Driver/convert.f90:1
+! Ensure argument -fconvert= works as expected.
+

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?



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>

peixin wrote:
> jpenix-quic wrote:
> > peixin wrote:
> > > jpenix-quic wrote:
> > > > peixin wrote:
> > > > > jpenix-quic wrote:
> > > > > > peixin wrote:
> > > > > > > Is it possible not to generated this global variable if 
> > > > > > > `fconvert=` is not specified?
> > > > > > 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?)
> > > > > 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?
> > > > 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!
> > > 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 

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

[Peixin Qiao via Phabricator via cfe-commits]

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>

jpenix-quic wrote:
> peixin wrote:
> > jpenix-quic wrote:
> > > peixin wrote:
> > > > jpenix-quic wrote:
> > > > > peixin wrote:
> > > > > > Is it possible not to generated this global variable if `fconvert=` 
> > > > > > is not specified?
> > > > > 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?)
> > > > 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?
> > > 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!
> > 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.
> 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 [[ 
> https://github.com/llvm/llvm-project/blob/c619d4f840dcba54751ff8c5aaafce0f173a4ad5/flang/runtime/environment.cpp#L50-L90
>  | here ]] that is responsible for initializing the runtime state so that 

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

[Jonathon Penix via Phabricator via cfe-commits]

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>

peixin wrote:
> jpenix-quic wrote:
> > peixin wrote:
> > > jpenix-quic wrote:
> > > > peixin wrote:
> > > > > Is it possible not to generated this global variable if `fconvert=` 
> > > > > is not specified?
> > > > 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?)
> > > 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?
> > 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!
> 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.
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 [[ 
https://github.com/llvm/llvm-project/blob/c619d4f840dcba54751ff8c5aaafce0f173a4ad5/flang/runtime/environment.cpp#L50-L90
 | 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 

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

[Peixin Qiao via Phabricator via cfe-commits]

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 security.
> 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 [[ 
> https://github.com/llvm/llvm-project/commit/824bf908194c9267f1f09065ba8e41d7969006ab
>  | 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?
Thanks for the explanations. The current fix makes sense to me.



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>

jpenix-quic wrote:
> peixin wrote:
> > jpenix-quic wrote:
> > > peixin wrote:
> > > > Is it possible not to generated this global variable if `fconvert=` is 
> > > > not specified?
> > > 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?)
> > 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?
> 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!
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 

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

[Jonathon Penix via Phabricator via cfe-commits]

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 
> possible in runtime for security.
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 [[ 
https://github.com/llvm/llvm-project/commit/824bf908194c9267f1f09065ba8e41d7969006ab
 | 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?



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>

peixin wrote:
> jpenix-quic wrote:
> > peixin wrote:
> > > Is it possible not to generated this global variable if `fconvert=` is 
> > > not specified?
> > 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?)
> 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?
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!


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonathon Penix via Phabricator via cfe-commits]

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
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/examples/external-hello.cpp
  flang/include/flang/Frontend/FrontendOptions.h
  flang/include/flang/Lower/Bridge.h
  flang/include/flang/Optimizer/Builder/Runtime/EnvironmentDefaults.h
  flang/include/flang/Runtime/environment-defaults.h
  flang/include/flang/Runtime/main.h
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/lib/Lower/Bridge.cpp
  flang/lib/Optimizer/Builder/CMakeLists.txt
  flang/lib/Optimizer/Builder/Runtime/EnvironmentDefaults.cpp
  flang/runtime/FortranMain/Fortran_main.c
  flang/runtime/environment-default-list.h
  flang/runtime/environment.cpp
  flang/runtime/environment.h
  flang/runtime/main.cpp
  flang/test/Driver/convert.f90
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-help.f90
  flang/test/Driver/emit-mlir.f90
  flang/test/Driver/frontend-forwarding.f90
  flang/test/Lower/convert.f90
  flang/test/Lower/environment-defaults.f90
  flang/test/Runtime/no-cpp-dep.c
  flang/tools/bbc/bbc.cpp
  flang/unittests/Runtime/CommandTest.cpp
  flang/unittests/Runtime/Stop.cpp

Index: flang/unittests/Runtime/Stop.cpp
===
--- flang/unittests/Runtime/Stop.cpp
+++ flang/unittests/Runtime/Stop.cpp
@@ -26,7 +26,8 @@
 
 TEST(TestProgramEnd, StopTestNoStopMessage) {
   putenv(const_cast("NO_STOP_MESSAGE=1"));
-  Fortran::runtime::executionEnvironment.Configure(0, nullptr, nullptr);
+  Fortran::runtime::executionEnvironment.Configure(
+  0, nullptr, nullptr, nullptr);
   EXPECT_EXIT(
   RTNAME(StopStatement)(), testing::ExitedWithCode(EXIT_SUCCESS), "");
 }
@@ -52,7 +53,8 @@
 
 TEST(TestProgramEnd, NoStopMessageTest) {
   putenv(const_cast("NO_STOP_MESSAGE=1"));
-  Fortran::runtime::executionEnvironment.Configure(0, nullptr, nullptr);
+  Fortran::runtime::executionEnvironment.Configure(
+  0, nullptr, nullptr, nullptr);
   static const char *message{"bye bye"};
   EXPECT_EXIT(RTNAME(StopStatementText)(message, std::strlen(message),
   /*isErrorStop=*/false, /*quiet=*/false),
Index: flang/unittests/Runtime/CommandTest.cpp
===
--- flang/unittests/Runtime/CommandTest.cpp
+++ flang/unittests/Runtime/CommandTest.cpp
@@ -49,7 +49,7 @@
 class CommandFixture : public ::testing::Test {
 protected:
   CommandFixture(int argc, const char *argv[]) {
-RTNAME(ProgramStart)(argc, argv, {});
+RTNAME(ProgramStart)(argc, argv, {}, {});
   }
 
   std::string GetPaddedStr(const char *text, std::size_t len) const {
Index: flang/tools/bbc/bbc.cpp
===
--- flang/tools/bbc/bbc.cpp
+++ flang/tools/bbc/bbc.cpp
@@ -222,7 +222,7 @@
   auto burnside = Fortran::lower::LoweringBridge::create(
   ctx, semanticsContext, defKinds, semanticsContext.intrinsics(),
   semanticsContext.targetCharacteristics(), parsing.allCooked(), "",
-  kindMap, loweringOptions);
+  kindMap, loweringOptions, {});
   burnside.lower(parseTree, semanticsContext);
   mlir::ModuleOp mlirModule = burnside.getModule();
   std::error_code ec;
Index: flang/test/Runtime/no-cpp-dep.c
===
--- flang/test/Runtime/no-cpp-dep.c
+++ flang/test/Runtime/no-cpp-dep.c
@@ -16,18 +16,20 @@
 we're testing. We can't include any headers directly since they likely contain
 C++ code that would explode here.
 */
+struct EnvironmentDefaultList;
 struct Descriptor;
 
 double RTNAME(CpuTime)();
 
-void RTNAME(ProgramStart)(int, const char *[], const char *[]);
+void RTNAME(ProgramStart)(
+int, const char *[], const char *[], const struct EnvironmentDefaultList *);
 int32_t RTNAME(ArgumentCount)();
 int32_t RTNAME(GetCommandArgument)(int32_t, const struct Descriptor *,
 const struct Descriptor *, const struct Descriptor *);
 
 int main() {
   double x = RTNAME(CpuTime)();
-  RTNAME(ProgramStart)(0, 0, 0);
+  RTNAME(ProgramStart)(0, 0, 0, 0);
   int32_t c = RTNAME(ArgumentCount)();
   int32_t v = RTNAME(GetCommandArgument)(0, 0, 0, 0);
   return x + c + v;
Index: flang/test/Lower/environment-defaults.f90
===
--- /dev/null
+++ flang/test/Lower/environment-defaults.f90
@@ -0,0 +1,11 @@
+! RUN: bbc -emit-fir -o - %s | FileCheck %s
+
+program test
+  continue
+end
+
+! Test that a null pointer is generated for environment defaults if nothing is specified
+
+! CHECK: fir.global @_QQEnvironmentDefaults constant : !fir.ref, !fir.ref> {
+! CHECK:  %[[VAL_0:.*]] = fir.zero_bits !fir.ref, !fir.ref>
+! CHECK: 

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

[Peixin Qiao via Phabricator via cfe-commits]

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: 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>

jpenix-quic wrote:
> peixin wrote:
> > Is it possible not to generated this global variable if `fconvert=` is not 
> > specified?
> 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?)
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?


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonathon Penix via Phabricator via cfe-commits]

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>

peixin wrote:
> Is it possible not to generated this global variable if `fconvert=` is not 
> specified?
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?)


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Peixin Qiao via Phabricator via cfe-commits]

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: flang/lib/Optimizer/Builder/Runtime/EnvironmentDefaults.cpp:19
+const std::vector ) {
+  std::string envDefaultListPtrName =
+  fir::NameUniquer::doEnvironmentDefaultList().str();

```
  if (builder.getNamedGlobal(envDefaultListPtrName))
return;
```
I don't think this and doEnvironmentDefaultList are necessary.



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>

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


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Peixin Qiao via Phabricator via cfe-commits]

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: 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 

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

[Jonathon Penix via Phabricator via cfe-commits]

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 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.
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?)


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonathon Penix via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D130513/new/

https://reviews.llvm.org/D130513

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/examples/external-hello.cpp
  flang/include/flang/Frontend/FrontendOptions.h
  flang/include/flang/Lower/Bridge.h
  flang/include/flang/Optimizer/Builder/Runtime/EnvironmentDefaults.h
  flang/include/flang/Optimizer/Support/InternalNames.h
  flang/include/flang/Runtime/environment-defaults.h
  flang/include/flang/Runtime/main.h
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/lib/Lower/Bridge.cpp
  flang/lib/Optimizer/Builder/CMakeLists.txt
  flang/lib/Optimizer/Builder/Runtime/EnvironmentDefaults.cpp
  flang/lib/Optimizer/Support/InternalNames.cpp
  flang/runtime/FortranMain/Fortran_main.c
  flang/runtime/environment-default-list.h
  flang/runtime/environment.cpp
  flang/runtime/environment.h
  flang/runtime/main.cpp
  flang/test/Driver/convert.f90
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-help.f90
  flang/test/Driver/emit-mlir.f90
  flang/test/Driver/frontend-forwarding.f90
  flang/test/Lower/convert.f90
  flang/test/Lower/environment-defaults.f90
  flang/test/Runtime/no-cpp-dep.c
  flang/tools/bbc/bbc.cpp
  flang/unittests/Runtime/CommandTest.cpp
  flang/unittests/Runtime/Stop.cpp

Index: flang/unittests/Runtime/Stop.cpp
===
--- flang/unittests/Runtime/Stop.cpp
+++ flang/unittests/Runtime/Stop.cpp
@@ -26,7 +26,8 @@
 
 TEST(TestProgramEnd, StopTestNoStopMessage) {
   putenv(const_cast("NO_STOP_MESSAGE=1"));
-  Fortran::runtime::executionEnvironment.Configure(0, nullptr, nullptr);
+  Fortran::runtime::executionEnvironment.Configure(
+  0, nullptr, nullptr, nullptr);
   EXPECT_EXIT(
   RTNAME(StopStatement)(), testing::ExitedWithCode(EXIT_SUCCESS), "");
 }
@@ -52,7 +53,8 @@
 
 TEST(TestProgramEnd, NoStopMessageTest) {
   putenv(const_cast("NO_STOP_MESSAGE=1"));
-  Fortran::runtime::executionEnvironment.Configure(0, nullptr, nullptr);
+  Fortran::runtime::executionEnvironment.Configure(
+  0, nullptr, nullptr, nullptr);
   static const char *message{"bye bye"};
   EXPECT_EXIT(RTNAME(StopStatementText)(message, std::strlen(message),
   /*isErrorStop=*/false, /*quiet=*/false),
Index: flang/unittests/Runtime/CommandTest.cpp
===
--- flang/unittests/Runtime/CommandTest.cpp
+++ flang/unittests/Runtime/CommandTest.cpp
@@ -49,7 +49,7 @@
 class CommandFixture : public ::testing::Test {
 protected:
   CommandFixture(int argc, const char *argv[]) {
-RTNAME(ProgramStart)(argc, argv, {});
+RTNAME(ProgramStart)(argc, argv, {}, {});
   }
 
   std::string GetPaddedStr(const char *text, std::size_t len) const {
Index: flang/tools/bbc/bbc.cpp
===
--- flang/tools/bbc/bbc.cpp
+++ flang/tools/bbc/bbc.cpp
@@ -222,7 +222,7 @@
   auto burnside = Fortran::lower::LoweringBridge::create(
   ctx, semanticsContext, defKinds, semanticsContext.intrinsics(),
   semanticsContext.targetCharacteristics(), parsing.allCooked(), "",
-  kindMap, loweringOptions);
+  kindMap, loweringOptions, {});
   burnside.lower(parseTree, semanticsContext);
   mlir::ModuleOp mlirModule = burnside.getModule();
   std::error_code ec;
Index: flang/test/Runtime/no-cpp-dep.c
===
--- flang/test/Runtime/no-cpp-dep.c
+++ flang/test/Runtime/no-cpp-dep.c
@@ -16,18 +16,20 @@
 we're testing. We can't include any headers directly since they likely contain
 C++ code that would explode here.
 */
+struct EnvironmentDefaultList;
 struct Descriptor;
 
 double RTNAME(CpuTime)();
 
-void RTNAME(ProgramStart)(int, const char *[], const char *[]);
+void RTNAME(ProgramStart)(
+int, const char *[], const char *[], const struct EnvironmentDefaultList *);
 int32_t RTNAME(ArgumentCount)();
 int32_t RTNAME(GetCommandArgument)(int32_t, const struct Descriptor *,
 const struct Descriptor *, const struct Descriptor *);
 
 int main() {
   double x = RTNAME(CpuTime)();
-  RTNAME(ProgramStart)(0, 0, 0);
+  RTNAME(ProgramStart)(0, 0, 0, 0);
   int32_t c = RTNAME(ArgumentCount)();
   int32_t v = RTNAME(GetCommandArgument)(0, 0, 0, 0);
   return x + c + v;
Index: flang/test/Lower/environment-defaults.f90
===
--- /dev/null
+++ flang/test/Lower/environment-defaults.f90
@@ -0,0 +1,11 @@
+! RUN: bbc -emit-fir -o - %s | FileCheck %s
+
+program test
+  continue
+end
+
+! Test that a null pointer is generated for environment defaults 

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

[Peixin Qiao via Phabricator via cfe-commits]

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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Peter Klausler via Phabricator via cfe-commits]

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 
===//
+//

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.


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonathon Penix via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonathon Penix via Phabricator via cfe-commits]

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:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/examples/external-hello.cpp
  flang/include/flang/Frontend/FrontendOptions.h
  flang/include/flang/Lower/Bridge.h
  flang/include/flang/Lower/Runtime.h
  flang/include/flang/Optimizer/Builder/Runtime/EnvironmentDefaults.h
  flang/include/flang/Optimizer/Support/InternalNames.h
  flang/include/flang/Runtime/environment-defaults.h
  flang/include/flang/Runtime/main.h
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/lib/Lower/Bridge.cpp
  flang/lib/Optimizer/Builder/CMakeLists.txt
  flang/lib/Optimizer/Builder/Runtime/EnvironmentDefaults.cpp
  flang/lib/Optimizer/Support/InternalNames.cpp
  flang/runtime/FortranMain/Fortran_main.c
  flang/runtime/environment-default-list.h
  flang/runtime/environment.cpp
  flang/runtime/environment.h
  flang/runtime/main.cpp
  flang/test/Driver/convert.f90
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-help.f90
  flang/test/Driver/emit-mlir.f90
  flang/test/Driver/frontend-forwarding.f90
  flang/test/Lower/convert.f90
  flang/test/Lower/environment-defaults.f90
  flang/test/Runtime/no-cpp-dep.c
  flang/tools/bbc/bbc.cpp
  flang/unittests/Runtime/CommandTest.cpp
  flang/unittests/Runtime/Stop.cpp

Index: flang/unittests/Runtime/Stop.cpp
===
--- flang/unittests/Runtime/Stop.cpp
+++ flang/unittests/Runtime/Stop.cpp
@@ -26,7 +26,8 @@
 
 TEST(TestProgramEnd, StopTestNoStopMessage) {
   putenv(const_cast("NO_STOP_MESSAGE=1"));
-  Fortran::runtime::executionEnvironment.Configure(0, nullptr, nullptr);
+  Fortran::runtime::executionEnvironment.Configure(
+  0, nullptr, nullptr, nullptr);
   EXPECT_EXIT(
   RTNAME(StopStatement)(), testing::ExitedWithCode(EXIT_SUCCESS), "");
 }
@@ -52,7 +53,8 @@
 
 TEST(TestProgramEnd, NoStopMessageTest) {
   putenv(const_cast("NO_STOP_MESSAGE=1"));
-  Fortran::runtime::executionEnvironment.Configure(0, nullptr, nullptr);
+  Fortran::runtime::executionEnvironment.Configure(
+  0, nullptr, nullptr, nullptr);
   static const char *message{"bye bye"};
   EXPECT_EXIT(RTNAME(StopStatementText)(message, std::strlen(message),
   /*isErrorStop=*/false, /*quiet=*/false),
Index: flang/unittests/Runtime/CommandTest.cpp
===
--- flang/unittests/Runtime/CommandTest.cpp
+++ flang/unittests/Runtime/CommandTest.cpp
@@ -49,7 +49,7 @@
 class CommandFixture : public ::testing::Test {
 protected:
   CommandFixture(int argc, const char *argv[]) {
-RTNAME(ProgramStart)(argc, argv, {});
+RTNAME(ProgramStart)(argc, argv, {}, {});
   }
 
   std::string GetPaddedStr(const char *text, std::size_t len) const {
Index: flang/tools/bbc/bbc.cpp
===
--- flang/tools/bbc/bbc.cpp
+++ flang/tools/bbc/bbc.cpp
@@ -222,7 +222,7 @@
   auto burnside = Fortran::lower::LoweringBridge::create(
   ctx, defKinds, semanticsContext.intrinsics(),
   semanticsContext.targetCharacteristics(), parsing.allCooked(), "",
-  kindMap, loweringOptions);
+  kindMap, loweringOptions, {});
   burnside.lower(parseTree, semanticsContext);
   mlir::ModuleOp mlirModule = burnside.getModule();
   std::error_code ec;
Index: flang/test/Runtime/no-cpp-dep.c
===
--- flang/test/Runtime/no-cpp-dep.c
+++ flang/test/Runtime/no-cpp-dep.c
@@ -16,18 +16,20 @@
 we're testing. We can't include any headers directly since they likely contain
 C++ code that would explode here.
 */
+struct EnvironmentDefaultList;
 struct Descriptor;
 
 double RTNAME(CpuTime)();
 
-void RTNAME(ProgramStart)(int, const char *[], const char *[]);
+void RTNAME(ProgramStart)(
+int, const char *[], const char *[], const struct EnvironmentDefaultList *);
 int32_t RTNAME(ArgumentCount)();
 int32_t RTNAME(GetCommandArgument)(int32_t, const struct Descriptor *,
 const struct Descriptor *, const struct Descriptor *);
 
 int main() {
   double x = RTNAME(CpuTime)();
-  RTNAME(ProgramStart)(0, 0, 0);
+  RTNAME(ProgramStart)(0, 0, 0, 0);
   int32_t c = RTNAME(ArgumentCount)();
   int32_t v = RTNAME(GetCommandArgument)(0, 0, 0, 0);
   return x + c + v;
Index: flang/test/Lower/environment-defaults.f90
===
--- /dev/null
+++ flang/test/Lower/environment-defaults.f90
@@ -0,0 +1,11 @@
+! RUN: bbc -emit-fir -o - %s | FileCheck %s
+
+program test
+  continue
+end
+
+! Test that a null pointer is generated for environment defaults if nothing is specified
+
+! CHECK: fir.global 

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

[Peixin Qiao via Phabricator via cfe-commits]

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 this can avoid add one variable to the bridge.
> 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!)
Yes.


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonathon Penix via Phabricator via cfe-commits]

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 to the bridge.
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!)


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonathon Penix via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D130513

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/examples/external-hello.cpp
  flang/include/flang/Frontend/FrontendOptions.h
  flang/include/flang/Lower/Bridge.h
  flang/include/flang/Lower/Runtime.h
  flang/include/flang/Optimizer/Builder/Runtime/EnvironmentDefaults.h
  flang/include/flang/Optimizer/Support/InternalNames.h
  flang/include/flang/Runtime/environment-defaults.h
  flang/include/flang/Runtime/main.h
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/lib/Lower/Bridge.cpp
  flang/lib/Optimizer/Builder/CMakeLists.txt
  flang/lib/Optimizer/Builder/Runtime/EnvironmentDefaults.cpp
  flang/lib/Optimizer/Support/InternalNames.cpp
  flang/runtime/FortranMain/Fortran_main.c
  flang/runtime/environment-default-list.h
  flang/runtime/environment.cpp
  flang/runtime/environment.h
  flang/runtime/main.cpp
  flang/test/Driver/convert.f90
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-help.f90
  flang/test/Driver/emit-mlir.f90
  flang/test/Driver/frontend-forwarding.f90
  flang/test/Lower/convert.f90
  flang/test/Lower/environment-defaults.f90
  flang/test/Runtime/no-cpp-dep.c
  flang/tools/bbc/bbc.cpp
  flang/unittests/Runtime/CommandTest.cpp
  flang/unittests/Runtime/Stop.cpp

Index: flang/unittests/Runtime/Stop.cpp
===
--- flang/unittests/Runtime/Stop.cpp
+++ flang/unittests/Runtime/Stop.cpp
@@ -26,7 +26,8 @@
 
 TEST(TestProgramEnd, StopTestNoStopMessage) {
   putenv(const_cast("NO_STOP_MESSAGE=1"));
-  Fortran::runtime::executionEnvironment.Configure(0, nullptr, nullptr);
+  Fortran::runtime::executionEnvironment.Configure(
+  0, nullptr, nullptr, nullptr);
   EXPECT_EXIT(
   RTNAME(StopStatement)(), testing::ExitedWithCode(EXIT_SUCCESS), "");
 }
@@ -52,7 +53,8 @@
 
 TEST(TestProgramEnd, NoStopMessageTest) {
   putenv(const_cast("NO_STOP_MESSAGE=1"));
-  Fortran::runtime::executionEnvironment.Configure(0, nullptr, nullptr);
+  Fortran::runtime::executionEnvironment.Configure(
+  0, nullptr, nullptr, nullptr);
   static const char *message{"bye bye"};
   EXPECT_EXIT(RTNAME(StopStatementText)(message, std::strlen(message),
   /*isErrorStop=*/false, /*quiet=*/false),
Index: flang/unittests/Runtime/CommandTest.cpp
===
--- flang/unittests/Runtime/CommandTest.cpp
+++ flang/unittests/Runtime/CommandTest.cpp
@@ -49,7 +49,7 @@
 class CommandFixture : public ::testing::Test {
 protected:
   CommandFixture(int argc, const char *argv[]) {
-RTNAME(ProgramStart)(argc, argv, {});
+RTNAME(ProgramStart)(argc, argv, {}, {});
   }
 
   std::string GetPaddedStr(const char *text, std::size_t len) const {
Index: flang/tools/bbc/bbc.cpp
===
--- flang/tools/bbc/bbc.cpp
+++ flang/tools/bbc/bbc.cpp
@@ -220,7 +220,7 @@
   auto burnside = Fortran::lower::LoweringBridge::create(
   ctx, defKinds, semanticsContext.intrinsics(),
   semanticsContext.targetCharacteristics(), parsing.allCooked(), "",
-  kindMap);
+  kindMap, {});
   burnside.lower(parseTree, semanticsContext);
   mlir::ModuleOp mlirModule = burnside.getModule();
   std::error_code ec;
Index: flang/test/Runtime/no-cpp-dep.c
===
--- flang/test/Runtime/no-cpp-dep.c
+++ flang/test/Runtime/no-cpp-dep.c
@@ -16,18 +16,20 @@
 we're testing. We can't include any headers directly since they likely contain
 C++ code that would explode here.
 */
+struct EnvironmentDefaultList;
 struct Descriptor;
 
 double RTNAME(CpuTime)();
 
-void RTNAME(ProgramStart)(int, const char *[], const char *[]);
+void RTNAME(ProgramStart)(
+int, const char *[], const char *[], const struct EnvironmentDefaultList *);
 int32_t RTNAME(ArgumentCount)();
 int32_t RTNAME(GetCommandArgument)(int32_t, const struct Descriptor *,
 const struct Descriptor *, const struct Descriptor *);
 
 int main() {
   double x = RTNAME(CpuTime)();
-  RTNAME(ProgramStart)(0, 0, 0);
+  RTNAME(ProgramStart)(0, 0, 0, 0);
   int32_t c = RTNAME(ArgumentCount)();
   int32_t v = RTNAME(GetCommandArgument)(0, 0, 0, 0);
   return x + c + v;
Index: flang/test/Lower/environment-defaults.f90
===
--- /dev/null
+++ flang/test/Lower/environment-defaults.f90
@@ -0,0 +1,12 @@
+! RUN: bbc -emit-fir -o - %s | FileCheck %s
+
+program test
+  continue
+end
+
+! Test that a null pointer is generated for environment defaults if nothing is specified
+

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

[Peixin Qiao via Phabricator via cfe-commits]

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.


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonathon Penix via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D130513

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/examples/external-hello.cpp
  flang/include/flang/Frontend/FrontendOptions.h
  flang/include/flang/Lower/Bridge.h
  flang/include/flang/Lower/Runtime.h
  flang/include/flang/Optimizer/Builder/Runtime/EnvironmentDefaults.h
  flang/include/flang/Optimizer/Support/InternalNames.h
  flang/include/flang/Runtime/environment-defaults.h
  flang/include/flang/Runtime/main.h
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/lib/Lower/Bridge.cpp
  flang/lib/Optimizer/Builder/CMakeLists.txt
  flang/lib/Optimizer/Builder/Runtime/EnvironmentDefaults.cpp
  flang/lib/Optimizer/Support/InternalNames.cpp
  flang/runtime/FortranMain/Fortran_main.c
  flang/runtime/environment-default-list.h
  flang/runtime/environment.cpp
  flang/runtime/environment.h
  flang/runtime/main.cpp
  flang/test/Driver/convert.f90
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-help.f90
  flang/test/Driver/emit-mlir.f90
  flang/test/Driver/frontend-forwarding.f90
  flang/test/Lower/convert.f90
  flang/test/Lower/environment-defaults.f90
  flang/test/Runtime/no-cpp-dep.c
  flang/tools/bbc/bbc.cpp
  flang/unittests/Runtime/CommandTest.cpp
  flang/unittests/Runtime/Stop.cpp

Index: flang/unittests/Runtime/Stop.cpp
===
--- flang/unittests/Runtime/Stop.cpp
+++ flang/unittests/Runtime/Stop.cpp
@@ -26,7 +26,8 @@
 
 TEST(TestProgramEnd, StopTestNoStopMessage) {
   putenv(const_cast("NO_STOP_MESSAGE=1"));
-  Fortran::runtime::executionEnvironment.Configure(0, nullptr, nullptr);
+  Fortran::runtime::executionEnvironment.Configure(
+  0, nullptr, nullptr, nullptr);
   EXPECT_EXIT(
   RTNAME(StopStatement)(), testing::ExitedWithCode(EXIT_SUCCESS), "");
 }
@@ -52,7 +53,8 @@
 
 TEST(TestProgramEnd, NoStopMessageTest) {
   putenv(const_cast("NO_STOP_MESSAGE=1"));
-  Fortran::runtime::executionEnvironment.Configure(0, nullptr, nullptr);
+  Fortran::runtime::executionEnvironment.Configure(
+  0, nullptr, nullptr, nullptr);
   static const char *message{"bye bye"};
   EXPECT_EXIT(RTNAME(StopStatementText)(message, std::strlen(message),
   /*isErrorStop=*/false, /*quiet=*/false),
Index: flang/unittests/Runtime/CommandTest.cpp
===
--- flang/unittests/Runtime/CommandTest.cpp
+++ flang/unittests/Runtime/CommandTest.cpp
@@ -49,7 +49,7 @@
 class CommandFixture : public ::testing::Test {
 protected:
   CommandFixture(int argc, const char *argv[]) {
-RTNAME(ProgramStart)(argc, argv, {});
+RTNAME(ProgramStart)(argc, argv, {}, {});
   }
 
   std::string GetPaddedStr(const char *text, std::size_t len) const {
Index: flang/tools/bbc/bbc.cpp
===
--- flang/tools/bbc/bbc.cpp
+++ flang/tools/bbc/bbc.cpp
@@ -220,7 +220,7 @@
   auto burnside = Fortran::lower::LoweringBridge::create(
   ctx, defKinds, semanticsContext.intrinsics(),
   semanticsContext.targetCharacteristics(), parsing.allCooked(), "",
-  kindMap);
+  kindMap, {});
   burnside.lower(parseTree, semanticsContext);
   mlir::ModuleOp mlirModule = burnside.getModule();
   std::error_code ec;
Index: flang/test/Runtime/no-cpp-dep.c
===
--- flang/test/Runtime/no-cpp-dep.c
+++ flang/test/Runtime/no-cpp-dep.c
@@ -16,18 +16,20 @@
 we're testing. We can't include any headers directly since they likely contain
 C++ code that would explode here.
 */
+struct EnvironmentDefaultList;
 struct Descriptor;
 
 double RTNAME(CpuTime)();
 
-void RTNAME(ProgramStart)(int, const char *[], const char *[]);
+void RTNAME(ProgramStart)(
+int, const char *[], const char *[], const struct EnvironmentDefaultList *);
 int32_t RTNAME(ArgumentCount)();
 int32_t RTNAME(GetCommandArgument)(int32_t, const struct Descriptor *,
 const struct Descriptor *, const struct Descriptor *);
 
 int main() {
   double x = RTNAME(CpuTime)();
-  RTNAME(ProgramStart)(0, 0, 0);
+  RTNAME(ProgramStart)(0, 0, 0, 0);
   int32_t c = RTNAME(ArgumentCount)();
   int32_t v = RTNAME(GetCommandArgument)(0, 0, 0, 0);
   return x + c + v;
Index: flang/test/Lower/environment-defaults.f90
===
--- /dev/null
+++ flang/test/Lower/environment-defaults.f90
@@ -0,0 +1,12 @@
+! RUN: bbc -emit-fir -o - %s | FileCheck %s
+
+program test
+  continue
+end
+
+! Test that a null pointer is generated for environment defaults if nothing is specified
+
+! CHECK: fir.global 

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

[Jonathon Penix via Phabricator via cfe-commits]

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 = bridge.getConversion();

peixin wrote:
> I think this should be moved into `void 
> lowerFunc(Fortran::lower::pft::FunctionLikeUnit )`.
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. 



Comment at: flang/runtime/main.cpp:55
+
+  if (auto convert{Fortran::runtime::GetConvertFromInt(i)}) {
+Fortran::runtime::executionEnvironment.conversion = *convert;

peixin wrote:
> Nit: Is it better to to check the range of i before getting it from 
> `GetConvertFromInt`?
> ```
> if (i < static_cast(Convert::Unknown) || i > 
> static_cast(Convert::Swap)) {
>   crash
> } else {
>   Fortran::runtime::executionEnvironment.conversion = static_cast(i);
> }
> ```
I ended up not needing `GetConvertFromInt` when reworking the approach, so this 
has been removed!



Comment at: flang/runtime/main.cpp:58
+  } else {
+Fortran::runtime::Terminator{__FILE__, __LINE__}.Crash(
+"Invalid convert option passed to ConvertOption");

peixin wrote:
> 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?
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.


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonathon Penix via Phabricator via cfe-commits]

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, arpith-jacob, 
nicolasvasilache, antiagainst, shauheen, rriddle.

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.


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

https://reviews.llvm.org/D130513

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/examples/external-hello.cpp
  flang/include/flang/Frontend/FrontendOptions.h
  flang/include/flang/Lower/Bridge.h
  flang/include/flang/Lower/Runtime.h
  flang/include/flang/Optimizer/Builder/Runtime/EnvironmentDefaults.h
  flang/include/flang/Optimizer/Support/InternalNames.h
  flang/include/flang/Runtime/environment-defaults.h
  flang/include/flang/Runtime/main.h
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/lib/Lower/Bridge.cpp
  flang/lib/Optimizer/Builder/CMakeLists.txt
  flang/lib/Optimizer/Builder/Runtime/EnvironmentDefaults.cpp
  flang/lib/Optimizer/Support/InternalNames.cpp
  flang/runtime/FortranMain/Fortran_main.c
  flang/runtime/environment-default-list.h
  flang/runtime/environment.cpp
  flang/runtime/environment.h
  flang/runtime/main.cpp
  flang/test/Driver/convert.f90
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-help.f90
  flang/test/Driver/emit-mlir.f90
  flang/test/Driver/frontend-forwarding.f90
  flang/test/Lower/convert.f90
  flang/test/Lower/environment-defaults.f90
  flang/test/Runtime/no-cpp-dep.c
  flang/tools/bbc/bbc.cpp
  flang/unittests/Runtime/CommandTest.cpp
  flang/unittests/Runtime/Stop.cpp

Index: flang/unittests/Runtime/Stop.cpp
===
--- flang/unittests/Runtime/Stop.cpp
+++ flang/unittests/Runtime/Stop.cpp
@@ -26,7 +26,8 @@
 
 TEST(TestProgramEnd, StopTestNoStopMessage) {
   putenv(const_cast("NO_STOP_MESSAGE=1"));
-  Fortran::runtime::executionEnvironment.Configure(0, nullptr, nullptr);
+  Fortran::runtime::executionEnvironment.Configure(0, nullptr, nullptr,
+   nullptr);
   EXPECT_EXIT(
   RTNAME(StopStatement)(), testing::ExitedWithCode(EXIT_SUCCESS), "");
 }
@@ -52,7 +53,8 @@
 
 TEST(TestProgramEnd, NoStopMessageTest) {
   putenv(const_cast("NO_STOP_MESSAGE=1"));
-  Fortran::runtime::executionEnvironment.Configure(0, nullptr, nullptr);
+  Fortran::runtime::executionEnvironment.Configure(0, nullptr, nullptr,
+   nullptr);
   static const char *message{"bye bye"};
   EXPECT_EXIT(RTNAME(StopStatementText)(message, std::strlen(message),
   /*isErrorStop=*/false, /*quiet=*/false),
Index: flang/unittests/Runtime/CommandTest.cpp
===
--- flang/unittests/Runtime/CommandTest.cpp
+++ flang/unittests/Runtime/CommandTest.cpp
@@ -49,7 +49,7 @@
 class CommandFixture : public ::testing::Test {
 protected:
   CommandFixture(int argc, const char *argv[]) {
-RTNAME(ProgramStart)(argc, argv, {});
+RTNAME(ProgramStart)(argc, argv, {}, {});
   }
 
   std::string GetPaddedStr(const char *text, std::size_t len) const {
Index: flang/tools/bbc/bbc.cpp
===
--- flang/tools/bbc/bbc.cpp
+++ flang/tools/bbc/bbc.cpp
@@ -220,7 +220,7 @@
   auto burnside = Fortran::lower::LoweringBridge::create(
   ctx, defKinds, semanticsContext.intrinsics(),
   semanticsContext.targetCharacteristics(), parsing.allCooked(), "",
-  kindMap);
+  kindMap, {});
   burnside.lower(parseTree, semanticsContext);
   mlir::ModuleOp mlirModule = burnside.getModule();
   std::error_code ec;
Index: flang/test/Runtime/no-cpp-dep.c
===
--- flang/test/Runtime/no-cpp-dep.c
+++ flang/test/Runtime/no-cpp-dep.c
@@ -16,18 +16,20 @@
 we're testing. We can't include any headers directly since they likely contain
 C++ code that would explode here.
 */
+struct EnvironmentDefaultList;
 struct Descriptor;
 
 double RTNAME(CpuTime)();
 
-void RTNAME(ProgramStart)(int, const char *[], const char *[]);
+void RTNAME(ProgramStart)(int, const char *[], const char *[],
+  const struct EnvironmentDefaultList *);
 int32_t RTNAME(ArgumentCount)();
 int32_t RTNAME(GetCommandArgument)(int32_t, const struct Descriptor *,
 const struct Descriptor *, const struct Descriptor *);
 
 int main() {
   double x = RTNAME(CpuTime)();
-  

[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, Alias;
+def fconvert_EQ : Joined<["-"], "fconvert=">, 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 )`.



Comment at: 

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

[Jonathon Penix via Phabricator via cfe-commits]

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 wrote:
> jpenix-quic wrote:
> > 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!
> 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!
I went the `AddOtherOptions` route for now. Apologies for the unrelated change 
and thank you for checking over this!


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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonathon Penix via Phabricator via cfe-commits]

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 SINCE LAST ACTION
  https://reviews.llvm.org/D130513/new/

https://reviews.llvm.org/D130513

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/include/flang/Frontend/FrontendOptions.h
  flang/include/flang/Lower/Bridge.h
  flang/include/flang/Optimizer/Builder/Runtime/Convert.h
  flang/include/flang/Runtime/convert.h
  flang/include/flang/Runtime/main.h
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/lib/Lower/Bridge.cpp
  flang/lib/Optimizer/Builder/CMakeLists.txt
  flang/lib/Optimizer/Builder/Runtime/Convert.cpp
  flang/runtime/environment.cpp
  flang/runtime/environment.h
  flang/runtime/main.cpp
  flang/test/Driver/convert.f90
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-help.f90
  flang/test/Driver/frontend-forwarding.f90
  flang/tools/bbc/bbc.cpp
  flang/unittests/Runtime/ExternalIOTest.cpp

Index: flang/unittests/Runtime/ExternalIOTest.cpp
===
--- flang/unittests/Runtime/ExternalIOTest.cpp
+++ flang/unittests/Runtime/ExternalIOTest.cpp
@@ -149,6 +149,67 @@
   << "EndIoStatement() for Close";
 }
 
+TEST(ExternalIOTests, TestDirectUnformattedSwappedConvert) {
+  // OPEN(NEWUNIT=unit,ACCESS='DIRECT',ACTION='READWRITE',&
+  //   FORM='UNFORMATTED',RECL=8,STATUS='SCRATCH',CONVERT='NATIVE')
+  auto *io{IONAME(BeginOpenNewUnit)(__FILE__, __LINE__)};
+  ASSERT_TRUE(IONAME(SetAccess)(io, "DIRECT", 6)) << "SetAccess(DIRECT)";
+  ASSERT_TRUE(IONAME(SetAction)(io, "READWRITE", 9)) << "SetAction(READWRITE)";
+  ASSERT_TRUE(IONAME(SetForm)(io, "UNFORMATTED", 11)) << "SetForm(UNFORMATTED)";
+  ASSERT_TRUE(IONAME(SetConvert)(io, "NATIVE", 6)) << "SetConvert(NATIVE)";
+
+  std::int64_t buffer;
+  static constexpr std::size_t recl{sizeof buffer};
+  ASSERT_TRUE(IONAME(SetRecl)(io, recl)) << "SetRecl()";
+  ASSERT_TRUE(IONAME(SetStatus)(io, "SCRATCH", 7)) << "SetStatus(SCRATCH)";
+
+  int unit{-1};
+  ASSERT_TRUE(IONAME(GetNewUnit)(io, unit)) << "GetNewUnit()";
+  ASSERT_EQ(IONAME(EndIoStatement)(io), IostatOk)
+  << "EndIoStatement() for OpenNewUnit";
+
+  static constexpr int records{10};
+  for (int j{1}; j <= records; ++j) {
+// WRITE(UNIT=unit,REC=j) j
+io = IONAME(BeginUnformattedOutput)(unit, __FILE__, __LINE__);
+ASSERT_TRUE(IONAME(SetRec)(io, j)) << "SetRec(" << j << ')';
+buffer = j;
+ASSERT_TRUE(IONAME(OutputUnformattedBlock)(
+io, reinterpret_cast(), recl, recl))
+<< "OutputUnformattedBlock()";
+ASSERT_EQ(IONAME(EndIoStatement)(io), IostatOk)
+<< "EndIoStatement() for OutputUnformattedBlock";
+  }
+
+  // Set unformatted conversion to SWAP
+  RTNAME(ConvertOption)(4);
+  // OPEN(UNIT=unit,STATUS='OLD')
+  io = IONAME(BeginOpenUnit)(unit, __FILE__, __LINE__);
+  ASSERT_TRUE(IONAME(SetStatus)(io, "OLD", 3)) << "SetStatus(OLD)";
+  ASSERT_EQ(IONAME(EndIoStatement)(io), IostatOk)
+  << "EndIoStatement() for OpenUnit";
+
+  for (int j{records}; j >= 1; --j) {
+// READ(UNIT=unit,REC=j) n
+io = IONAME(BeginUnformattedInput)(unit, __FILE__, __LINE__);
+ASSERT_TRUE(IONAME(SetRec)(io, j)) << "SetRec(" << j << ')';
+ASSERT_TRUE(IONAME(InputUnformattedBlock)(
+io, reinterpret_cast(), recl, recl))
+<< "InputUnformattedBlock()";
+ASSERT_EQ(IONAME(EndIoStatement)(io), IostatOk)
+<< "EndIoStatement() for InputUnformattedBlock";
+ASSERT_EQ(buffer >> 56, j)
+<< "Read back " << (buffer >> 56) << " from direct unformatted record "
+<< j << ", expected " << j << '\n';
+  }
+
+  // CLOSE(UNIT=unit,STATUS='DELETE')
+  io = IONAME(BeginClose)(unit, __FILE__, __LINE__);
+  ASSERT_TRUE(IONAME(SetStatus)(io, "DELETE", 6)) << "SetStatus(DELETE)";
+  ASSERT_EQ(IONAME(EndIoStatement)(io), IostatOk)
+  << "EndIoStatement() for Close";
+}
+
 TEST(ExternalIOTests, TestSequentialFixedUnformatted) {
   // OPEN(NEWUNIT=unit,ACCESS='SEQUENTIAL',ACTION='READWRITE',&
   //   FORM='UNFORMATTED',RECL=8,STATUS='SCRATCH')
Index: flang/tools/bbc/bbc.cpp
===
--- flang/tools/bbc/bbc.cpp
+++ flang/tools/bbc/bbc.cpp
@@ -33,6 +33,7 @@
 #include "flang/Parser/parsing.h"
 #include "flang/Parser/provenance.h"
 #include "flang/Parser/unparse.h"
+#include "flang/Runtime/convert.h"
 #include "flang/Semantics/expression.h"
 #include "flang/Semantics/runtime-type-info.h"
 #include "flang/Semantics/semantics.h"
@@ -219,7 +220,7 @@
   auto burnside = Fortran::lower::LoweringBridge::create(
   ctx, defKinds, semanticsContext.intrinsics(),
   semanticsContext.targetCharacteristics(), 

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

[Andrzej Warzynski via Phabricator via cfe-commits]

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, Alias;
+def fconvert_EQ : Joined<["-"], "fconvert=">, Group,
+  HelpText<"Set endian conversion of data for unformatted files">;

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.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:52
+options::OPT_fno_automatic,
+options::OPT_fconvert_EQ});
 }

jpenix-quic wrote:
> 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!
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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonathon Penix via Phabricator via cfe-commits]

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 unformatted files">;

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!



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:52
+options::OPT_fno_automatic,
+options::OPT_fconvert_EQ});
 }

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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Peixin Qiao via Phabricator via cfe-commits]

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 unformatted files">;

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonathon Penix via Phabricator via cfe-commits]

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 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Peter Klausler via Phabricator via cfe-commits]

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Peter Klausler via Phabricator via cfe-commits]

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 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonathon Penix via Phabricator via cfe-commits]

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). 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonathon Penix via Phabricator via cfe-commits]

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 this revision.
Herald added subscribers: cfe-commits, sstefan1, MaskRay.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.

This follows gfortran's approach of generating a runtime call to set
the conversion state for the entire program and takes effect only if
the fconvert option is used on the main program (as the runtime call
is inserted into _QQmain).

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130513

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/include/flang/Frontend/FrontendOptions.h
  flang/include/flang/Lower/Bridge.h
  flang/include/flang/Optimizer/Builder/Runtime/Convert.h
  flang/include/flang/Runtime/convert.h
  flang/include/flang/Runtime/main.h
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/lib/Lower/Bridge.cpp
  flang/lib/Optimizer/Builder/CMakeLists.txt
  flang/lib/Optimizer/Builder/Runtime/Convert.cpp
  flang/runtime/environment.cpp
  flang/runtime/environment.h
  flang/runtime/main.cpp
  flang/test/Driver/convert.f90
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-help.f90
  flang/test/Driver/frontend-forwarding.f90
  flang/tools/bbc/bbc.cpp
  flang/unittests/Runtime/ExternalIOTest.cpp

Index: flang/unittests/Runtime/ExternalIOTest.cpp
===
--- flang/unittests/Runtime/ExternalIOTest.cpp
+++ flang/unittests/Runtime/ExternalIOTest.cpp
@@ -149,6 +149,67 @@
   << "EndIoStatement() for Close";
 }
 
+TEST(ExternalIOTests, TestDirectUnformattedSwappedConvert) {
+  // OPEN(NEWUNIT=unit,ACCESS='DIRECT',ACTION='READWRITE',&
+  //   FORM='UNFORMATTED',RECL=8,STATUS='SCRATCH',CONVERT='NATIVE')
+  auto *io{IONAME(BeginOpenNewUnit)(__FILE__, __LINE__)};
+  ASSERT_TRUE(IONAME(SetAccess)(io, "DIRECT", 6)) << "SetAccess(DIRECT)";
+  ASSERT_TRUE(IONAME(SetAction)(io, "READWRITE", 9)) << "SetAction(READWRITE)";
+  ASSERT_TRUE(IONAME(SetForm)(io, "UNFORMATTED", 11)) << "SetForm(UNFORMATTED)";
+  ASSERT_TRUE(IONAME(SetConvert)(io, "NATIVE", 6)) << "SetConvert(NATIVE)";
+
+  std::int64_t buffer;
+  static constexpr std::size_t recl{sizeof buffer};
+  ASSERT_TRUE(IONAME(SetRecl)(io, recl)) << "SetRecl()";
+  ASSERT_TRUE(IONAME(SetStatus)(io, "SCRATCH", 7)) << "SetStatus(SCRATCH)";
+
+  int unit{-1};
+  ASSERT_TRUE(IONAME(GetNewUnit)(io, unit)) << "GetNewUnit()";
+  ASSERT_EQ(IONAME(EndIoStatement)(io), IostatOk)
+  << "EndIoStatement() for OpenNewUnit";
+
+  static constexpr int records{10};
+  for (int j{1}; j <= records; ++j) {
+// WRITE(UNIT=unit,REC=j) j
+io = IONAME(BeginUnformattedOutput)(unit, __FILE__, __LINE__);
+ASSERT_TRUE(IONAME(SetRec)(io, j)) << "SetRec(" << j << ')';
+buffer = j;
+ASSERT_TRUE(IONAME(OutputUnformattedBlock)(
+io, reinterpret_cast(), recl, recl))
+<< "OutputUnformattedBlock()";
+ASSERT_EQ(IONAME(EndIoStatement)(io), IostatOk)
+<< "EndIoStatement() for OutputUnformattedBlock";
+  }
+
+  // Set unformatted conversion to SWAP
+  RTNAME(ConvertOption)(4);
+  // OPEN(UNIT=unit,STATUS='OLD')
+  io = IONAME(BeginOpenUnit)(unit, __FILE__, __LINE__);
+  ASSERT_TRUE(IONAME(SetStatus)(io, "OLD", 3)) << "SetStatus(OLD)";
+  ASSERT_EQ(IONAME(EndIoStatement)(io), IostatOk)
+  << "EndIoStatement() for OpenUnit";
+
+  for (int j{records}; j >= 1; --j) {
+// READ(UNIT=unit,REC=j) n
+io = IONAME(BeginUnformattedInput)(unit, __FILE__, __LINE__);
+ASSERT_TRUE(IONAME(SetRec)(io, j)) << "SetRec(" << j << ')';
+ASSERT_TRUE(IONAME(InputUnformattedBlock)(
+io, reinterpret_cast(), recl, recl))
+<< "InputUnformattedBlock()";
+ASSERT_EQ(IONAME(EndIoStatement)(io), IostatOk)
+<< "EndIoStatement() for InputUnformattedBlock";
+ASSERT_EQ(buffer >> 56, j)
+<< "Read back " << (buffer >> 56) << " from direct unformatted record "
+<< j << ", expected " << j << '\n';
+  }
+
+  // CLOSE(UNIT=unit,STATUS='DELETE')
+  io = IONAME(BeginClose)(unit, __FILE__, __LINE__);
+  ASSERT_TRUE(IONAME(SetStatus)(io, "DELETE", 6)) << "SetStatus(DELETE)";
+  ASSERT_EQ(IONAME(EndIoStatement)(io), IostatOk)
+  << "EndIoStatement() for Close";
+}
+
 TEST(ExternalIOTests, TestSequentialFixedUnformatted) {
   // OPEN(NEWUNIT=unit,ACCESS='SEQUENTIAL',ACTION='READWRITE',&
   //   FORM='UNFORMATTED',RECL=8,STATUS='SCRATCH')
Index: flang/tools/bbc/bbc.cpp
===
--- flang/tools/bbc/bbc.cpp
+++ flang/tools/bbc/bbc.cpp
@@ -33,6 +33,7 @@
 #include "flang/Parser/parsing.h"
 #include