Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flang][runtime] Build ISO_FORTRAN_ENV to export kind arrays as linkable symbols #95388

Merged
merged 11 commits into from
Jul 19, 2024

Conversation

mjklemm
Copy link
Contributor

@mjklemm mjklemm commented Jun 13, 2024

Moves definitions of the kind arrays into a Fortran MODULE to not only emit the MOD file, but also compile that MODULE file into an object file. This file is then linked into libFortranRuntime.so.

Eventually this workaround PR shoud be redone and a proper runtime build should be setup that will then also compile Fortran MODULE files.

Fixes #89403

@mjklemm mjklemm requested a review from klausler June 13, 2024 10:46
@mjklemm mjklemm self-assigned this Jun 13, 2024
@llvmbot llvmbot added flang:driver flang:runtime flang Flang issues not falling into any other category labels Jun 13, 2024
@mjklemm mjklemm marked this pull request as draft June 13, 2024 10:47
@llvmbot
Copy link
Collaborator

llvmbot commented Jun 13, 2024

@llvm/pr-subscribers-flang-runtime

@llvm/pr-subscribers-flang-driver

Author: Michael Klemm (mjklemm)

Changes

Moves definitions of the kind arrays into a Fortran MODULE to not only emit the MOD file, but also compile that MODULE file into an object file. This file is then linked into libFortranRuntime.so.

Eventually this workaround PR shoud be redone and a proper runtime build should be setup that will then also compile Fortran MODULE files.

Fixes #89403


Full diff: https://github.com/llvm/llvm-project/pull/95388.diff

6 Files Affected:

  • (modified) flang/CMakeLists.txt (+1)
  • (added) flang/module/__fortran_builtin_kinds.f90 (+110)
  • (modified) flang/module/iso_fortran_env.f90 (+31-87)
  • (modified) flang/runtime/CMakeLists.txt (+1)
  • (modified) flang/tools/f18/CMakeLists.txt (+39-3)
  • (modified) flang/tools/flang-driver/CMakeLists.txt (-2)
diff --git a/flang/CMakeLists.txt b/flang/CMakeLists.txt
index cbe8f1186236a..070c39eb6e9ab 100644
--- a/flang/CMakeLists.txt
+++ b/flang/CMakeLists.txt
@@ -538,3 +538,4 @@ get_clang_resource_dir(HEADER_INSTALL_DIR SUBDIR include)
 install(
   FILES include/flang/ISO_Fortran_binding.h
   DESTINATION ${HEADER_INSTALL_DIR} )
+
diff --git a/flang/module/__fortran_builtin_kinds.f90 b/flang/module/__fortran_builtin_kinds.f90
new file mode 100644
index 0000000000000..189055dc26c0c
--- /dev/null
+++ b/flang/module/__fortran_builtin_kinds.f90
@@ -0,0 +1,110 @@
+!===-- module/__fortran_bultin_kinds.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
+!
+!===------------------------------------------------------------------------===!
+
+module __fortran_builtin_kinds
+  implicit none
+  private
+
+  ! INTEGER types
+  integer, parameter, public :: &
+    selectedInt8 = selected_int_kind(2), &
+    selectedInt16 = selected_int_kind(4), &
+    selectedInt32 = selected_int_kind(9), &
+    selectedInt64 = selected_int_kind(18),&
+    selectedInt128 = selected_int_kind(38), &
+    safeInt8 = merge(selectedInt8, selected_int_kind(0), &
+                     selectedInt8 >= 0), &
+    safeInt16 = merge(selectedInt16, selected_int_kind(0), &
+                      selectedInt16 >= 0), &
+    safeInt32 = merge(selectedInt32, selected_int_kind(0), &
+                      selectedInt32 >= 0), &
+    safeInt64 = merge(selectedInt64, selected_int_kind(0), &
+                      selectedInt64 >= 0), &
+    safeInt128 = merge(selectedInt128, selected_int_kind(0), &
+                       selectedInt128 >= 0)
+
+  integer, parameter, public :: &
+    int8 = merge(selectedInt8, merge(-2, -1, selectedInt8 >= 0), &
+                 digits(int(0,kind=safeInt8)) == 7), &
+    int16 = merge(selectedInt16, merge(-2, -1, selectedInt16 >= 0), &
+                  digits(int(0,kind=safeInt16)) == 15), &
+    int32 = merge(selectedInt32, merge(-2, -1, selectedInt32 >= 0), &
+                  digits(int(0,kind=safeInt32)) == 31), &
+    int64 = merge(selectedInt64, merge(-2, -1, selectedInt64 >= 0), &
+                  digits(int(0,kind=safeInt64)) == 63), &
+    int128 = merge(selectedInt128, merge(-2, -1, selectedInt128 >= 0), &
+                   digits(int(0,kind=safeInt128)) == 127)
+
+  integer, parameter, dimension(*), public :: __builtin_integer_kinds = [ &
+      selected_int_kind(0), &
+      [(pack([selected_int_kind(k)], &
+             selected_int_kind(k) >= 0 .and. &
+               selected_int_kind(k) /= selected_int_kind(k-1)), &
+        integer :: k=1, 39)]]
+
+  ! LOGICAL TYPES
+  integer, parameter, public :: &
+    logical8 = int8, logical16 = int16, logical32 = int32, logical64 = int64
+
+  integer, parameter, dimension(*), public :: __builtin_logical_kinds = [ &
+      pack([logical8],  logical8 >= 0), &
+      pack([logical16], logical16 >= 0), &
+      pack([logical32], logical32 >= 0), &
+      pack([logical64], logical64 >= 0) &
+    ]
+
+  ! REAL types
+  integer, parameter, public :: &
+    selectedReal16 = selected_real_kind(3, 4), &      ! IEEE half
+    selectedBfloat16 = selected_real_kind(2, 37), &   ! truncated IEEE single
+    selectedReal32 = selected_real_kind(6, 37), &     ! IEEE single
+    selectedReal64 = selected_real_kind(15, 307), &   ! IEEE double
+    selectedReal80 = selected_real_kind(18, 4931), &  ! 80x87 extended
+    selectedReal64x2 = selected_real_kind(31, 307), & ! "double-double"
+    selectedReal128 = selected_real_kind(33, 4931), & ! IEEE quad
+    safeReal16 = merge(selectedReal16, selected_real_kind(0,0), &
+                       selectedReal16 >= 0), &
+    safeBfloat16 = merge(selectedBfloat16, selected_real_kind(0,0), &
+                         selectedBfloat16 >= 0), &
+    safeReal32 = merge(selectedReal32, selected_real_kind(0,0), &
+                       selectedReal32 >= 0), &
+    safeReal64 = merge(selectedReal64, selected_real_kind(0,0), &
+                       selectedReal64 >= 0), &
+    safeReal80 = merge(selectedReal80, selected_real_kind(0,0), &
+                       selectedReal80 >= 0), &
+    safeReal64x2 = merge(selectedReal64x2, selected_real_kind(0,0), &
+                         selectedReal64x2 >= 0), &
+    safeReal128 = merge(selectedReal128, selected_real_kind(0,0), &
+                        selectedReal128 >= 0)
+
+  integer, parameter, public :: &
+    real16 = merge(selectedReal16, merge(-2, -1, selectedReal16 >= 0), &
+                   digits(real(0,kind=safeReal16)) == 11), &
+    bfloat16 = merge(selectedBfloat16, merge(-2, -1, selectedBfloat16 >= 0), &
+                     digits(real(0,kind=safeBfloat16)) == 8), &
+    real32 = merge(selectedReal32, merge(-2, -1, selectedReal32 >= 0), &
+                   digits(real(0,kind=safeReal32)) == 24), &
+    real64 = merge(selectedReal64, merge(-2, -1, selectedReal64 >= 0), &
+                   digits(real(0,kind=safeReal64)) == 53), &
+    real80 = merge(selectedReal80, merge(-2, -1, selectedReal80 >= 0), &
+                   digits(real(0,kind=safeReal80)) == 64), &
+    real64x2 = merge(selectedReal64x2, merge(-2, -1, selectedReal64x2 >= 0), &
+                     digits(real(0,kind=safeReal64x2)) == 106), &
+    real128 = merge(selectedReal128, merge(-2, -1, selectedReal128 >= 0), &
+                    digits(real(0,kind=safeReal128)) == 113)
+
+  integer, parameter, dimension(*), public :: __builtin_real_kinds = [ &
+      pack([real16], real16 >= 0), &
+      pack([bfloat16], bfloat16 >= 0), &
+      pack([real32], real32 >= 0), &
+      pack([real64], real64 >= 0), &
+      pack([real80], real80 >= 0), &
+      pack([real64x2], real64x2 >= 0), &
+      pack([real128], real128 >= 0) &
+    ]
+end module __fortran_builtin_kinds
\ No newline at end of file
diff --git a/flang/module/iso_fortran_env.f90 b/flang/module/iso_fortran_env.f90
index 6ca98e518aeac..f751ba03f7bac 100644
--- a/flang/module/iso_fortran_env.f90
+++ b/flang/module/iso_fortran_env.f90
@@ -22,6 +22,23 @@ module iso_fortran_env
     compiler_options => __builtin_compiler_options, &
     compiler_version => __builtin_compiler_version
 
+  use __fortran_builtin_kinds, only: &
+    selectedInt8, selectedInt16, selectedInt32, selectedInt64, selectedInt128, &
+    safeInt8, safeInt16, safeInt32, safeInt64, safeInt128, &
+    int8, int16, int32, int64, int128, &
+    logical8, logical16, logical32, logical64, &
+    selectedReal16, selectedBfloat16, selectedReal32, &
+    selectedReal64, selectedReal80, selectedReal64x2, &
+    selectedReal128, &
+    safeReal16, safeBfloat16, safeReal32, &
+    safeReal64, safeReal80, safeReal64x2, &
+    safeReal128, &
+    real16, bfloat16, real32, real64, &
+    real80, real64x2, real128, &
+    integer_kinds => __builtin_integer_kinds, &
+    real_kinds => __builtin_real_kinds, &
+    logical_kinds => __builtin_logical_kinds
+
   implicit none
   private
 
@@ -38,95 +55,22 @@ module iso_fortran_env
     pack([selectedUCS_2], selectedUCS_2 >= 0), &
     pack([selectedUnicode], selectedUnicode >= 0)]
 
-  integer, parameter :: &
-    selectedInt8 = selected_int_kind(2), &
-    selectedInt16 = selected_int_kind(4), &
-    selectedInt32 = selected_int_kind(9), &
-    selectedInt64 = selected_int_kind(18),&
-    selectedInt128 = selected_int_kind(38), &
-    safeInt8 = merge(selectedInt8, selected_int_kind(0), &
-                     selectedInt8 >= 0), &
-    safeInt16 = merge(selectedInt16, selected_int_kind(0), &
-                      selectedInt16 >= 0), &
-    safeInt32 = merge(selectedInt32, selected_int_kind(0), &
-                      selectedInt32 >= 0), &
-    safeInt64 = merge(selectedInt64, selected_int_kind(0), &
-                      selectedInt64 >= 0), &
-    safeInt128 = merge(selectedInt128, selected_int_kind(0), &
-                       selectedInt128 >= 0)
-  integer, parameter, public :: &
-    int8 = merge(selectedInt8, merge(-2, -1, selectedInt8 >= 0), &
-                 digits(int(0,kind=safeInt8)) == 7), &
-    int16 = merge(selectedInt16, merge(-2, -1, selectedInt16 >= 0), &
-                  digits(int(0,kind=safeInt16)) == 15), &
-    int32 = merge(selectedInt32, merge(-2, -1, selectedInt32 >= 0), &
-                  digits(int(0,kind=safeInt32)) == 31), &
-    int64 = merge(selectedInt64, merge(-2, -1, selectedInt64 >= 0), &
-                  digits(int(0,kind=safeInt64)) == 63), &
-    int128 = merge(selectedInt128, merge(-2, -1, selectedInt128 >= 0), &
-                   digits(int(0,kind=safeInt128)) == 127)
-
-  integer, parameter, public :: integer_kinds(*) = [ &
-    selected_int_kind(0), &
-    [(pack([selected_int_kind(k)], &
-           selected_int_kind(k) >= 0 .and. &
-             selected_int_kind(k) /= selected_int_kind(k-1)), &
-      integer :: k=1, 39)]]
+  public :: selectedInt8, selectedInt16, selectedInt32, selectedInt64, selectedInt128, &
+    safeInt8, safeInt16, safeInt32, safeInt64, safeInt128, &
+    int8, int16, int32, int64, int128
 
-  integer, parameter, public :: &
-    logical8 = int8, logical16 = int16, logical32 = int32, logical64 = int64
-  integer, parameter, public :: logical_kinds(*) = [ &
-    pack([logical8],  logical8 >= 0), &
-    pack([logical16], logical16 >= 0), &
-    pack([logical32], logical32 >= 0), &
-    pack([logical64], logical64 >= 0)]
+  public :: logical8, logical16, logical32, logical64
 
-  integer, parameter :: &
-    selectedReal16 = selected_real_kind(3, 4), &      ! IEEE half
-    selectedBfloat16 = selected_real_kind(2, 37), &   ! truncated IEEE single
-    selectedReal32 = selected_real_kind(6, 37), &     ! IEEE single
-    selectedReal64 = selected_real_kind(15, 307), &   ! IEEE double
-    selectedReal80 = selected_real_kind(18, 4931), &  ! 80x87 extended
-    selectedReal64x2 = selected_real_kind(31, 307), & ! "double-double"
-    selectedReal128 = selected_real_kind(33, 4931), & ! IEEE quad
-    safeReal16 = merge(selectedReal16, selected_real_kind(0,0), &
-                       selectedReal16 >= 0), &
-    safeBfloat16 = merge(selectedBfloat16, selected_real_kind(0,0), &
-                         selectedBfloat16 >= 0), &
-    safeReal32 = merge(selectedReal32, selected_real_kind(0,0), &
-                       selectedReal32 >= 0), &
-    safeReal64 = merge(selectedReal64, selected_real_kind(0,0), &
-                       selectedReal64 >= 0), &
-    safeReal80 = merge(selectedReal80, selected_real_kind(0,0), &
-                       selectedReal80 >= 0), &
-    safeReal64x2 = merge(selectedReal64x2, selected_real_kind(0,0), &
-                         selectedReal64x2 >= 0), &
-    safeReal128 = merge(selectedReal128, selected_real_kind(0,0), &
-                        selectedReal128 >= 0)
-  integer, parameter, public :: &
-    real16 = merge(selectedReal16, merge(-2, -1, selectedReal16 >= 0), &
-                   digits(real(0,kind=safeReal16)) == 11), &
-    bfloat16 = merge(selectedBfloat16, merge(-2, -1, selectedBfloat16 >= 0), &
-                     digits(real(0,kind=safeBfloat16)) == 8), &
-    real32 = merge(selectedReal32, merge(-2, -1, selectedReal32 >= 0), &
-                   digits(real(0,kind=safeReal32)) == 24), &
-    real64 = merge(selectedReal64, merge(-2, -1, selectedReal64 >= 0), &
-                   digits(real(0,kind=safeReal64)) == 53), &
-    real80 = merge(selectedReal80, merge(-2, -1, selectedReal80 >= 0), &
-                   digits(real(0,kind=safeReal80)) == 64), &
-    real64x2 = merge(selectedReal64x2, merge(-2, -1, selectedReal64x2 >= 0), &
-                     digits(real(0,kind=safeReal64x2)) == 106), &
-    real128 = merge(selectedReal128, merge(-2, -1, selectedReal128 >= 0), &
-                    digits(real(0,kind=safeReal128)) == 113)
-
-  integer, parameter, public :: real_kinds(*) = [ &
-    pack([real16], real16 >= 0), &
-    pack([bfloat16], bfloat16 >= 0), &
-    pack([real32], real32 >= 0), &
-    pack([real64], real64 >= 0), &
-    pack([real80], real80 >= 0), &
-    pack([real64x2], real64x2 >= 0), &
-    pack([real128], real128 >= 0)]
+  public :: selectedReal16, selectedBfloat16, selectedReal32, &
+    selectedReal64, selectedReal80, selectedReal64x2, &
+    selectedReal128, &
+    safeReal16, safeBfloat16, safeReal32, &
+    safeReal64, safeReal80, safeReal64x2, &
+    safeReal128, &
+    real16, bfloat16, real32, real64, &
+    real80, real64x2, real128
+
+  public :: integer_kinds, real_kinds, logical_kinds
 
   integer, parameter, public :: current_team = -1, &
     initial_team = -2, &
diff --git a/flang/runtime/CMakeLists.txt b/flang/runtime/CMakeLists.txt
index a826980e19411..c3bec4043e760 100644
--- a/flang/runtime/CMakeLists.txt
+++ b/flang/runtime/CMakeLists.txt
@@ -260,6 +260,7 @@ if (NOT DEFINED MSVC)
 
     INSTALL_WITH_TOOLCHAIN
   )
+  target_link_libraries(FortranRuntime PRIVATE ${FORTRAN_MODULE_OBJECTS})
 else()
   add_flang_library(FortranRuntime
     ${sources}
diff --git a/flang/tools/f18/CMakeLists.txt b/flang/tools/f18/CMakeLists.txt
index 4771199651602..5f4c49e42c36e 100644
--- a/flang/tools/f18/CMakeLists.txt
+++ b/flang/tools/f18/CMakeLists.txt
@@ -4,7 +4,16 @@ set(LLVM_LINK_COMPONENTS
   Support
   )
 
-set(MODULES
+# Define the list of Fortran module files that need to be compiled
+# to produce an object file for inclusion into the FortranRuntime
+# library.
+set(MODULES_WITH_IMPLEMENTATION
+  "__fortran_builtin_kinds"
+)
+
+# Define the list of Fortran module files for which it is
+# sufficient to generate the module file via -fsyntax-only.
+set(MODULES_WITHOUT_IMPLEMENTATION
   "__fortran_builtins"
   "__fortran_ieee_exceptions"
   "__fortran_type_info"
@@ -20,6 +29,12 @@ set(MODULES
   "iso_fortran_env"
 )
 
+set(MODULES ${MODULES_WITH_IMPLEMENTATION} ${MODULES_WITHOUT_IMPLEMENTATION})
+
+# Init variable to hold extra object files coming from the Fortran modules;
+# these module files will be contributed from the CMakeLists in flang/tools/f18.
+unset(FORTRAN_MODULE_OBJECTS CACHE)
+
 # Create module files directly from the top-level module source directory.
 # If CMAKE_CROSSCOMPILING, then the newly built flang-new executable was
 # cross compiled, and thus can't be executed on the build system and thus
@@ -41,6 +56,9 @@ if (NOT CMAKE_CROSSCOMPILING)
       if(NOT ${filename} STREQUAL "__fortran_type_info")
         set(depends ${depends} ${FLANG_INTRINSIC_MODULES_DIR}/__fortran_type_info.mod)
       endif()
+      if(${filename} STREQUAL "iso_fortran_env")
+        set(depends ${depends} ${FLANG_INTRINSIC_MODULES_DIR}/__fortran_builtin_kinds.mod)
+      endif()
       if(${filename} STREQUAL "ieee_arithmetic" OR
          ${filename} STREQUAL "ieee_exceptions")
         set(depends ${depends} ${FLANG_INTRINSIC_MODULES_DIR}/__fortran_ieee_exceptions.mod)
@@ -58,16 +76,34 @@ if (NOT CMAKE_CROSSCOMPILING)
       endif()
     endif()
 
+
+    # Some modules have an implementation part that needs to be added to the
+    # FortranRuntime library.
+    set(compile_with "-fsyntax-only")
+    set(object_output "")
+    set(include_in_link FALSE)
+    if(${filename} IN_LIST MODULES_WITH_IMPLEMENTATION)
+      set(compile_with "-c")
+      set(object_output "${CMAKE_CURRENT_BINARY_DIR}/${filename}${CMAKE_CXX_OUTPUT_EXTENSION}")
+      set(include_in_link TRUE)
+    endif()
+
     set(base ${FLANG_INTRINSIC_MODULES_DIR}/${filename})
     # TODO: We may need to flag this with conditional, in case Flang is built w/o OpenMP support
-    add_custom_command(OUTPUT ${base}.mod
+    add_custom_command(OUTPUT ${base}.mod ${object_output}
       COMMAND ${CMAKE_COMMAND} -E make_directory ${FLANG_INTRINSIC_MODULES_DIR}
-      COMMAND flang-new ${opts} -cpp -fsyntax-only -module-dir ${FLANG_INTRINSIC_MODULES_DIR}
+      COMMAND flang-new ${opts} -cpp ${compile_with} -module-dir ${FLANG_INTRINSIC_MODULES_DIR}
         ${FLANG_SOURCE_DIR}/module/${filename}.f90
       DEPENDS flang-new ${FLANG_SOURCE_DIR}/module/${filename}.f90 ${FLANG_SOURCE_DIR}/module/__fortran_builtins.f90 ${depends}
     )
     list(APPEND MODULE_FILES ${base}.mod)
     install(FILES ${base}.mod DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/flang")
+
+    # If a module has been compiled into an object file, add the file to
+    # the link line for the FortranRuntime library.
+    if(include_in_link)
+      set(FORTRAN_MODULE_OBJECTS "${FORTRAN_MODULE_OBJECTS}" "${object_output}" CACHE INTERNAL "")
+    endif()
   endforeach()
 
   # Special case for omp_lib.mod, because its source comes from openmp/runtime/src/include.
diff --git a/flang/tools/flang-driver/CMakeLists.txt b/flang/tools/flang-driver/CMakeLists.txt
index ce30ecff028dc..5bf535b18ad78 100644
--- a/flang/tools/flang-driver/CMakeLists.txt
+++ b/flang/tools/flang-driver/CMakeLists.txt
@@ -19,8 +19,6 @@ add_flang_tool(flang-new
   # These libraries are used in the linker invocation generated by the driver
   # (i.e. when constructing the linker job). Without them the driver would be
   # unable to generate executables.
-  FortranRuntime
-  FortranDecimal
 )
 
 target_link_libraries(flang-new

@@ -0,0 +1,110 @@
!===-- module/__fortran_bultin_kinds.f90 --=--------------------------------===!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why can't these definitions remain in iso_fortran_env?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flang was giving me compiler errors for numeric_storage_size, which seems it needs to not be compiled as part of the module file but rather as part of when the module has been included into the host routine.

That's I basically went back to my original idea of how I wanted to fix the bug and moved the intrinsic definitions into their own module file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__builtin_numeric_storage_size() gets special treatment in folding that can easily be adjusted without this kind of disruption.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide a hint how this could be done? I'm not that deep into the inner workings for Flang yet to see how this can be done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grep for builtin_numeric_storage and you'll see what's going on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In flang/lib/Evaluate/fold-integer.cpp ca. line 1340 is the code that folds (underscore underscore) builtin_numeric_storage. It defers that folding today until the reference to that intrinsic function is coming from a module file, at which point the size of a default numeric storage unit must be known. If you're changing things so that ISO_FORTRAN_ENV is now going to be compiled with the size of a default numeric storage unit being well-defined, then you can probably just remove the code that defers the folding.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I remove the code, the compiler folds the current value 32_4 into the iso_fortran_env.mod file. From there on, numeric_storage_size always reports 32 as the storage size, regardless of compiler options.

The code removed:

    if (!context.moduleFileName()) {
      // Don't fold this reference until it appears in the module file
      // for ISO_FORTRAN_ENV -- the value depends on the compiler options
      // that might be in force.
    } else {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but since you now need to also compile the modules into distinct *.o files for each combination of target and relevant compilation options, isn't that what has to happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not needed, if we do the separation like I'm proposing in this PR. Everything that is compiled into an object file is independent of the relevant compiler options (like -fdefault-real-8), while numeric_storage_size is left in iso_fortran_env that is only treated with -fsyntax-only, which resolves the whole issue w/o touching the compiler itself.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And complicated module file shenanigans are better somehow than "touching" the compiler? I guess that's a matter of opinion. I'll let you find some other reviewer for this, but you can help them out by at least documenting what's going on here with some comments, and fixing the name of the file in its header.

@clementval
Copy link
Contributor

clementval commented Jun 13, 2024

Did you move the definitions because you get an error compiling the full iso_fortran_env.f90 file? I think we can just wrap the part used during folding in a macro and not include it when we build the object file.

This is the part of the module I'm talking about:

  intrinsic :: __builtin_numeric_storage_size
  ! This value depends on any -fdefault-integer-N and -fdefault-real-N
  ! compiler options that are active when the module file is read.
  integer, parameter, public :: numeric_storage_size = &
    __builtin_numeric_storage_size()

@sscalpone
Copy link
Contributor

Thanks for looking at this, @mjklemm ! My thought was to move the build of the module files to be part of the runtime build. One advantage is that the object file can be put into the libFortranRuntime. The other is that the module file build is more closely associated with the target instead of the host.

@mjklemm
Copy link
Contributor Author

mjklemm commented Jun 14, 2024

I consider this a first step to resolve the bug, albeit with a workaround. In the long run, I totally agree that the Fortran runtime part should be built like any other LLVM runtime.

As part of that the module file should be considered to move to be within in the runtime folder and also the CMake for it should move from tools/f18 into the runtime folder. I think that would greatly simplify the CMake code needed.

@mjklemm
Copy link
Contributor Author

mjklemm commented Jun 14, 2024

Did you move the definitions because you get an error compiling the full iso_fortran_env.f90 file? I think we can just wrap the part used during folding in a macro and not include it when we build the object file.

This is the part of the module I'm talking about:


  intrinsic :: __builtin_numeric_storage_size

  ! This value depends on any -fdefault-integer-N and -fdefault-real-N

  ! compiler options that are active when the module file is read.

  integer, parameter, public :: numeric_storage_size = &

    __builtin_numeric_storage_size()

Yes, exactly. I think we can leave this as is if we feel that moving the compiled part of iso_fortran_env into its own module file like I did is the right thing.

Not sure if we would need this for other stuff in the future, but maybe splitting the intrinsic modules into an implementation file and a definitions file might be something to consider going forward.

Moves definitions of the kind arrays into a Fortran MODULE to not only
emit the MOD file, but also compile that MODULE file into an object
file.  This file is then linked into libFortranRuntime.so.

Eventually this workaround PR shoud be redone and a proper runtime build
should be setup that will then also compile Fortran MODULE files.
mjklemm and others added 2 commits July 16, 2024 20:01
Co-authored-by: Valentin Clement (バレンタイン クレメン) <clementval@gmail.com>
Co-authored-by: Michael Kruse <github@meinersbur.de>
@mjklemm
Copy link
Contributor Author

mjklemm commented Jul 17, 2024

I have made the requested updates.

@mjklemm
Copy link
Contributor Author

mjklemm commented Jul 18, 2024

@kkwli @Meinersbur @clementval Ping :-)

Copy link
Member

@Meinersbur Meinersbur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with two nits.

flang/tools/f18/CMakeLists.txt Outdated Show resolved Hide resolved
flang/module/iso_fortran_env_impl.f90 Outdated Show resolved Hide resolved
@Meinersbur
Copy link
Member

Btw, you should remove the draft status so flang category subscribers are notified

@mjklemm mjklemm marked this pull request as ready for review July 18, 2024 11:39
mjklemm and others added 4 commits July 18, 2024 15:05
Co-authored-by: Michael Kruse <github@meinersbur.de>
The test compiles and links a short program that uses the three kind
arrays.  It is supposed to fail and link time if the symbols are not
provided in the runtime.
Copy link
Collaborator

@kkwli kkwli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG

This reverts commit 04f52d6.
@mjklemm mjklemm merged commit 2f8b64d into llvm:main Jul 19, 2024
7 checks passed
@@ -169,6 +169,7 @@ set(sources
unit-map.cpp
unit.cpp
utf.cpp
${FORTRAN_MODULE_OBJECTS}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks some of my builds. I believe this won't work in general unless you make sure that the object file is built before FortranRuntime target tries to build, i.e. there has to be a target dependency in this CMake file.

I think you can try to reproduce it by deleting the module object file and running make FortranRuntime - it will fail with something like:

No rule to make target `tools/flang/tools/f18/iso_fortran_env_impl.o', needed by `lib/libFortranRuntime.a'.  Stop.

Copy link
Contributor Author

@mjklemm mjklemm Jul 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On it. Seems like Ninja understands the dependency, as it has a single build file that see the generated object file in one place and defers the target in the runtime libraries until that object file has been built. This does not seem to be the case for Makefile builds, because they are split across different Makefiles.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #99737 to hopefully resolve this. Can you please see if the PR resolves the issue?

And sorry for breaking your builds!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, Michael. No problem, it should fix my builds, and building module_files target before the complete build was a workaround for me.

mjklemm added a commit that referenced this pull request Jul 22, 2024
…ew (#99737)

Makefile-based builds did not have proper dependencies to built the
FortranRuntime target after Flang new is available. This PR introduces a
dependency to ensure that this is the case. Relates to PR #95388.

---------

Co-authored-by: Michael Kruse <github@meinersbur.de>
sgundapa pushed a commit to sgundapa/upstream_effort that referenced this pull request Jul 23, 2024
…ble symbols (llvm#95388)

Moves definitions of the kind arrays into a Fortran MODULE to not only
emit the MOD file, but also compile that MODULE file into an object
file. This file is then linked into libFortranRuntime.so.

Eventually this workaround PR shoud be redone and a proper runtime build
should be setup that will then also compile Fortran MODULE files.

Fixes llvm#89403

---------

Co-authored-by: Valentin Clement (バレンタイン クレメン) <clementval@gmail.com>
Co-authored-by: Michael Kruse <github@meinersbur.de>
sgundapa pushed a commit to sgundapa/upstream_effort that referenced this pull request Jul 23, 2024
…ew (llvm#99737)

Makefile-based builds did not have proper dependencies to built the
FortranRuntime target after Flang new is available. This PR introduces a
dependency to ensure that this is the case. Relates to PR llvm#95388.

---------

Co-authored-by: Michael Kruse <github@meinersbur.de>
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
…ble symbols (#95388)

Summary:
Moves definitions of the kind arrays into a Fortran MODULE to not only
emit the MOD file, but also compile that MODULE file into an object
file. This file is then linked into libFortranRuntime.so.

Eventually this workaround PR shoud be redone and a proper runtime build
should be setup that will then also compile Fortran MODULE files.

Fixes #89403

---------

Co-authored-by: Valentin Clement (バレンタイン クレメン) <clementval@gmail.com>
Co-authored-by: Michael Kruse <github@meinersbur.de>

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251666
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
…ew (#99737)

Makefile-based builds did not have proper dependencies to built the
FortranRuntime target after Flang new is available. This PR introduces a
dependency to ensure that this is the case. Relates to PR #95388.

---------

Co-authored-by: Michael Kruse <github@meinersbur.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:driver flang:runtime flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[flang] Linker for non-constant accesses to kind arrays (integer_kind, logical_kind, real_kind)
8 participants