[Flang] Adjust preprocessing to build modules correctly

[PeteSteinfeld]

Summary

Several module files in .../llvm-project/flang/module check for the
existence of the macro "x86_64" to conditionally compile Fortran
code. Unfortunately, this macro was not being defined anywhere. This
patch fixes that for compilations targeting 64 bit x86 machines.

I made the following changes --

• Removed the test for 32 bit X86 targets. The rest of the compiler and runtime do not support X86 32 bits.
• Added a predefined macro to define "x86_64" to 1 when the target architecture is 64 bit x86 and the "-cpp" option is on the command line.
• Changed the cmake file for creating the Fortran module files to use the "-cpp" option so that the macro "x86_64" will be defined when building the module files.

[sscalpone]

Hi Pete!

Looks good. I noticed that clang also generates some additional symbols:

#define __amd64 1
#define __amd64__ 1

These are available on Intel and AMD hardware just like x86_64.

I don't have a reason to keep the 32-bit code; however, others might.

[PeteSteinfeld]

In D135810#3853792, @sscalpone wrote:

Hi Pete!

Looks good. I noticed that clang also generates some additional symbols:

#define __amd64 1
#define __amd64__ 1

These are available on Intel and AMD hardware just like x86_64.

I don't have a reason to keep the 32-bit code; however, others might.

Also, __x86_64. I'll add them.

[PeteSteinfeld]

Adding other macro definitions associated with x86 architecture.

[kiranchandramohan]

Was wondering whether we can have a test for this.

flang/lib/Frontend/CompilerInvocation.cpp

863–869

Should we do the same for other platforms in the if or additional else cases?

[awarzynski]

Thanks for sending this, Pete! Could you add some tests? If you're looking for inspiration, predefined-macros-compiler-version.F90 could be a good starting point.

Also, from the summary:

Removed the test for 32 bit X86 targets.

No tests are removed in this patch.

Added a predefined macro to define "x86_64" to 1 when the target architecture is 64 bit x86 and the "-cpp" option is on the command line.

Shouldn't this macro be always defined? What do gfortran and clang do in this case?

On a more general note - it sounds like you are considering disabling support for 32 bit X86 targets. Makes sense, but it's worth documenting - I've seen folks asking about it and it's always unclear to me what the long term goal is.

flang/lib/Frontend/CompilerInvocation.cpp

862–869

Wouldn't setDefaultPredefinitions be more suitable for this?

flang/tools/f18/CMakeLists.txt

42

Why is -cpp needed here?

[PeteSteinfeld]

Moved the code for defining the new macros to the function
setDefaultPredefinitions, restricted the macros to the ones needed to fix the
problem at hand, and added a test.

[PeteSteinfeld]

In D135810#3855077, @awarzynski wrote:

Thanks for sending this, Pete! Could you add some tests? If you're looking for inspiration, predefined-macros-compiler-version.F90 could be a good starting point.

Thanks. I'll add a test.

Also, from the summary:

Removed the test for 32 bit X86 targets.

No tests are removed in this patch.

Sorry. I was unclear. I didn't remove any compiler tests. Rather, I removed the check for 32 bit x86 targets to determine whether to enable support for 80 bit REAL.

Added a predefined macro to define "x86_64" to 1 when the target architecture is 64 bit x86 and the "-cpp" option is on the command line.

Shouldn't this macro be always defined? What do gfortran and clang do in this case?

gfortran only runs its preprocessor when the -cpp option is used. clang always seems to run its preprocessor and define its predefined macros.

On a more general note - it sounds like you are considering disabling support for 32 bit X86 targets. Makes sense, but it's worth documenting - I've seen folks asking about it and it's always unclear to me what the long term goal is.

Yes. It's unclear to me what targets we want to support. This should be a larger discussion.

flang/lib/Frontend/CompilerInvocation.cpp

862–869

Sounds right. I'll move the code.

863–869

I think we need to have a larger conversation about what targets we want to support. But the changes in this patch are needed to generate correct module files for 64 bit X86 targets. Adding more targets is beyond the scope of what I'm trying to accomplish with this patch.

flang/tools/f18/CMakeLists.txt

42

The -cpp option is required to cause the predefined macros to be defined.

[awarzynski]

Thanks for your reply, Pete! You will need to to a small rebase before landing this - I just landed https://reviews.llvm.org/D130078. Sorry about that!

In D135810#3856751, @PeteSteinfeld wrote:

Shouldn't this macro be always defined? What do gfortran and clang do in this case?

gfortran only runs its preprocessor when the -cpp option is used. clang always seems to run its preprocessor and define its predefined macros.

It's a bit more convoluted. clang is not really involved here at all. LLVM Flang always runs it's preprocessor. However, in order to create an impression that the user is in control (and also to align with gfortran), flang-new supports -cpp and -nocpp and takes the file extension capitalisation into account. From gfortran docs:

The capitalized versions of either form are run through preprocessing.

The module files in LLVM Flang use .f90 and this means that macro pre-defines are "hidden" when these files are compiled through flang-new. Unless -cpp is used :) I hope that I got this right!

I've made a small suggestion, but otherwise LGTM, thanks!

flang/test/Driver/predefined-macros-x86.f90

5

This test should run just fine on darwin, macos and aarch64 provided that the X86 backened is enabled. And that's verified with ! REQUIRES: x86-registered-target. I think that you can delete this line.

[PeteSteinfeld]

Tweaked the test as suggested by @awarzynski.

[PeteSteinfeld]

Thanks for all of the feedback and guidance, Andrzej!

flang/test/Driver/predefined-macros-x86.f90

5

Thanks. I'll delete it.

[peixin]

flang/lib/Frontend/CompilerInvocation.cpp

859

Thanks for fixing this. Should this be fixed, too?

This was added since the codegen supports it (https://github.com/llvm/llvm-project/blob/25915c6ad2cb248b99fd61015e34fa9e819397b3/flang/lib/Optimizer/CodeGen/Target.cpp#L345-L357).

Does this fix mean that 32-bit machine won't be supported and the following code development need not to consider 32-bit machine?

[PeteSteinfeld]

@peixin , with this patch, I was only trying to fix the problem of building modules that support 80 bit REALs. I wasn't trying to make a bigger statement about which targets we should or should not support.

If someone thinks that support of 32 bit X86 REALs is important and they have the ability to test the associated code, I see no reason why they shouldn't implement it.

[peixin]

In D135810#3862223, @PeteSteinfeld wrote:

@peixin , with this patch, I was only trying to fix the problem of building modules that support 80 bit REALs. I wasn't trying to make a bigger statement about which targets we should or should not support.

If someone thinks that support of 32 bit X86 REALs is important and they have the ability to test the associated code, I see no reason why they shouldn't implement it.

OK. Got it. Thanks.

flang/lib/Frontend/CompilerInvocation.cpp

void CompilerInvocation::setDefaultPredefinitions() {
   if (frontendOptions.features.IsEnabled(
           Fortran::common::LanguageFeature::OpenACC)) {
     fortranOptions.predefinitions.emplace_back("_OPENACC", "202011");
   }
   if (frontendOptions.features.IsEnabled(
           Fortran::common::LanguageFeature::OpenMP)) {
     fortranOptions.predefinitions.emplace_back("_OPENMP", "201511");
   }
+  llvm::Triple targetTriple{llvm::Triple(this->targetOpts.triple)};
+  if (targetTriple.getArch() == llvm::Triple::ArchType::x86_64) {
+    fortranOptions.predefinitions.emplace_back("__x86_64__", "1");
+    fortranOptions.predefinitions.emplace_back("__x86_64", "1");
+  }
 }
 
 void CompilerInvocation::setFortranOpts() {
   auto &fortranOptions = getFortranOpts();
   const auto &frontendOptions = getFrontendOpts();
   const auto &preprocessorOptions = getPreprocessorOpts();
   auto &moduleDirJ = getModuleDir();
 
void CompilerInvocation::setFortranOpts() {
 
   if (getEnableConformanceChecks()) {
     fortranOptions.features.WarnOnAllNonstandard();
   }
 }
 
 void CompilerInvocation::setSemanticsOpts(
     Fortran::parser::AllCookedSources &allCookedSources) {
-  const auto &fortranOptions = getFortranOpts();
+  auto &fortranOptions = getFortranOpts();
 
   semanticsContext = std::make_unique<semantics::SemanticsContext>(
       getDefaultKinds(), fortranOptions.features, allCookedSources);
 
   semanticsContext->set_moduleDirectory(getModuleDir())
       .set_searchDirectories(fortranOptions.searchDirectories)
       .set_intrinsicModuleDirectories(fortranOptions.intrinsicModuleDirectories)
       .set_warnOnNonstandardUsage(getEnableConformanceChecks())
       .set_warningsAreErrors(getWarnAsErr())
       .set_moduleFileSuffix(getModuleFileSuffix());
 
   llvm::Triple targetTriple{llvm::Triple(this->targetOpts.triple)};
   // FIXME: Handle real(3) ?
-  if (targetTriple.getArch() != llvm::Triple::ArchType::x86 &&
-      targetTriple.getArch() != llvm::Triple::ArchType::x86_64) {
+  if (targetTriple.getArch() != llvm::Triple::ArchType::x86_64) {
     semanticsContext->targetCharacteristics().DisableType(
         Fortran::common::TypeCategory::Real, /*kind=*/10);
   }
 }
 
 /// Set \p loweringOptions controlling lowering behavior based
 /// on the \p optimizationLevel.
 void CompilerInvocation::setLoweringOptions() {
   const auto &codegenOpts = getCodeGenOpts();
 
   // Lower TRANSPOSE as a runtime call under -O0.
   loweringOpts.setOptimizeTranspose(codegenOpts.OptimizationLevel > 0);
 }

flang/test/Driver/predefined-macros-x86.f90

+! Test predefined macro for 64 bit X86 architecture
+
+! REQUIRES: x86-registered-target
+
+! RUN: %flang_fc1 -triple x86_64-unknown-linux-gnu -cpp -E %s | FileCheck %s
+
+! CHECK: integer :: var1 = 1
+! CHECK: integer :: var2 = 1
+
+#if __x86_64__
+  integer :: var1 = __x86_64__
+#endif
+#if __x86_64__
+  integer :: var2 = __x86_64
+#endif
+end program

flang/tools/f18/CMakeLists.txt

else()
       endif()
       if(${filename} STREQUAL "ieee_arithmetic" OR
          ${filename} STREQUAL "ieee_exceptions")
         set(depends ${FLANG_INTRINSIC_MODULES_DIR}/__fortran_ieee_exceptions.mod)
       endif()
     endif()
     add_custom_command(OUTPUT ${base}.mod
       COMMAND ${CMAKE_COMMAND} -E make_directory ${FLANG_INTRINSIC_MODULES_DIR}
-      COMMAND flang-new -fc1 -fsyntax-only -module-dir ${FLANG_INTRINSIC_MODULES_DIR}
+      COMMAND flang-new -fc1 -cpp -fsyntax-only -module-dir ${FLANG_INTRINSIC_MODULES_DIR}
         ${FLANG_SOURCE_DIR}/module/${filename}.f90
       DEPENDS flang-new ${FLANG_SOURCE_DIR}/module/${filename}.f90 ${depends}
     )
     add_custom_command(OUTPUT ${base}.f18.mod
       DEPENDS ${base}.mod
       COMMAND ${CMAKE_COMMAND} -E copy ${base}.mod ${base}.f18.mod)
     list(APPEND MODULE_FILES ${base}.mod ${base}.f18.mod)
     install(FILES ${base}.mod ${base}.f18.mod DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/flang")