[PATCH] D105811: [flang][driver] Delete `f18` (i.e. the old Flang driver)

Tue Jul 20 10:57:04 PDT 2021

kiranchandramohan added a comment.

I get some build errors like the following with this patch.

  c++ -DGTEST_HAS_RTTI=0 -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/flang/lib/Frontend -I/home/kircha02/llvm-project/flang/lib/Frontend -I/home/kircha02/llvm-project/flang/include -Itools/flang/include -Iinclude -I/home/kircha02/llvm-project/llvm/include -isystem /home/kircha02/llvm-project/llvm/../mlir/include -isystem tools/mlir/include -isystem tools/clang/include -isystem /home/kircha02/llvm-project/llvm/../clang/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -Wno-deprecated-copy -fno-strict-aliasing -fno-semantic-interposition -O3 -DNDEBUG  -fno-exceptions -fno-rtti -std=c++17 -MD -MT tools/flang/lib/Frontend/CMakeFiles/obj.flangFrontend.dir/CompilerInvocation.cpp.o -MF tools/flang/lib/Frontend/CMakeFiles/obj.flangFrontend.dir/CompilerInvocation.cpp.o.d -o tools/flang/lib/Frontend/CMakeFiles/obj.flangFrontend.dir/CompilerInvocation.cpp.o -c /home/kircha02/llvm-project/flang/lib/Frontend/CompilerInvocation.cpp
  /home/kircha02/llvm-project/flang/lib/Frontend/CompilerInvocation.cpp: In function 'bool ParseFrontendArgs(Fortran::frontend::FrontendOptions&, llvm::opt::ArgList&, clang::DiagnosticsEngine&)':
  /home/kircha02/llvm-project/flang/lib/Frontend/CompilerInvocation.cpp:105:36: error: 'class clang::DiagnosticsEngine' has no member named 'getNumErrors'; did you mean 'unsigned int clang::DiagnosticsEngine::NumErrors'? (not accessible from this context)
    105 |   unsigned numErrorsBefore = diags.getNumErrors();

Build steps used,

  cmake -G Ninja ../llvm -DLLVM_ENABLE_PROJECTS="flang;clang;mlir;openmp" -DLLVM_TARGETS_TO_BUILD=AArch64 -DCMAKE_BUILD_TYPE=Release
  nice -n 10 ninja

Since @sscalpone is the original author of the throw-away driver. It would be great to get his opinion before deleting f18.cpp. @klausler expressed some reservations in removing f18.cpp but subsequently said it is OK to delete it in the llvm-project/flang repo.

Regarding the dependency on Clang:

1. AMD engineers raised some issues in slack regarding build time in flang-compiler slack. Could @kiranktp let us know whether they have issues or whether they are OK?
2. I am guessing the default assumption is that the dependency on Clang will be permanent and there are no plans to remove the dependency.
3. One of the reasons for making the dependency on Clang permanent is some requirement for OpenMP that @jdoerfert  mentioned in the call. I am personally not aware of any dependencies as of now for OpenMP on clang. I am assuming that this has something to do with offloading. Will this cause MLIR to have a dependency on Clang too (which might run into issues with the MLIR team)? Or is this again a driver induced dependency?

================
Comment at: flang/include/flang/Frontend/CompilerInvocation.h:187

-  /// Set the Fortran options to predifined defaults. These defaults are
-  /// consistend with f18/f18.cpp.
+  /// Set the Fortran options to predifined defaults.
   // TODO: We should map frontendOpts_ to parserOpts_ instead. For that, we
----------------
spelling: predefined

================
Comment at: flang/test/lit.cfg.py:43
 # config.
-if config.include_flang_new_driver_test:
-  config.available_features.add('new-flang-driver')
-else:
-  config.available_features.add('old-flang-driver')
+config.available_features.add('new-flang-driver')

----------------
Is this required?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105811