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] Introduce FortranSupport #122069

Conversation

Meinersbur
Copy link
Member

Split off FortranSupport (analogous to LLVMSupport) from FortranCommon such that

  • declarations and definitions that are only used by the Flang compiler, but not by the runtime, are moved to FortranSupport

  • declarations and definitions that are used by both ("common"), the compiler and the runtime, remain in FortranCommon

This allows a for cleaner separation between compiler and runtime components, which are compiled differently. For instance, runtime sources must not use STL's <optional> which causes problems with CUDA support. Instead, the surrogate header flang/Common/optional.h must be used. This PR fixes this for fast-int-sel.h.

Declarations in include/Runtime are also used by both, but are header-only. ISO_Fortran_binding_wrapper.h, a header used by compiler and runtime, is also moved into FortranCommon.

The PR is split off from #110298 and #110217.

Copy link

github-actions bot commented Jan 8, 2025

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff f2abcde01601c91d3530ad47a34f98bd3eaaadc9 32e5b84a57d734fe6403addd50424aaf4f876efa --extensions h,cpp -- flang/include/flang/Common/fast-int-set.h flang/include/flang/Evaluate/call.h flang/include/flang/Evaluate/characteristics.h flang/include/flang/Evaluate/common.h flang/include/flang/Evaluate/constant.h flang/include/flang/Evaluate/expression.h flang/include/flang/Evaluate/formatting.h flang/include/flang/Evaluate/intrinsics.h flang/include/flang/Evaluate/shape.h flang/include/flang/Evaluate/target.h flang/include/flang/Evaluate/tools.h flang/include/flang/Evaluate/traverse.h flang/include/flang/Evaluate/type.h flang/include/flang/Evaluate/variable.h flang/include/flang/Frontend/CompilerInvocation.h flang/include/flang/Frontend/FrontendOptions.h flang/include/flang/ISO_Fortran_binding.h flang/include/flang/Lower/AbstractConverter.h flang/include/flang/Lower/Bridge.h flang/include/flang/Lower/CallInterface.h flang/include/flang/Lower/ConvertType.h flang/include/flang/Lower/LoweringOptions.h flang/include/flang/Lower/PFTBuilder.h flang/include/flang/Lower/Support/Utils.h flang/include/flang/Lower/SymbolMap.h flang/include/flang/Optimizer/Builder/FIRBuilder.h flang/include/flang/Optimizer/Builder/PPCIntrinsicCall.h flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h flang/include/flang/Optimizer/CodeGen/DescriptorModel.h flang/include/flang/Optimizer/Dialect/CUF/Attributes/CUFAttr.h flang/include/flang/Optimizer/Support/TypeCode.h flang/include/flang/Optimizer/Support/Utils.h flang/include/flang/Parser/char-block.h flang/include/flang/Parser/dump-parse-tree.h flang/include/flang/Parser/message.h flang/include/flang/Parser/parse-state.h flang/include/flang/Parser/parse-tree.h flang/include/flang/Parser/parsing.h flang/include/flang/Parser/provenance.h flang/include/flang/Parser/source.h flang/include/flang/Parser/user-state.h flang/include/flang/Runtime/descriptor-consts.h flang/include/flang/Runtime/descriptor.h flang/include/flang/Runtime/random.h flang/include/flang/Runtime/support.h flang/include/flang/Runtime/type-code.h flang/include/flang/Semantics/expression.h flang/include/flang/Semantics/runtime-type-info.h flang/include/flang/Semantics/scope.h flang/include/flang/Semantics/semantics.h flang/include/flang/Semantics/symbol.h flang/include/flang/Semantics/tools.h flang/include/flang/Semantics/type.h flang/include/flang/Support/Timing.h flang/include/flang/Tools/CrossToolHelpers.h flang/lib/Evaluate/call.cpp flang/lib/Evaluate/characteristics.cpp flang/lib/Evaluate/fold-implementation.h flang/lib/Evaluate/formatting.cpp flang/lib/Evaluate/intrinsics-library.cpp flang/lib/Evaluate/intrinsics.cpp flang/lib/Evaluate/shape.cpp flang/lib/Evaluate/target.cpp flang/lib/Frontend/CompilerInstance.cpp flang/lib/Frontend/CompilerInvocation.cpp flang/lib/Frontend/FrontendActions.cpp flang/lib/Lower/Bridge.cpp flang/lib/Lower/CallInterface.cpp flang/lib/Lower/ConvertExpr.cpp flang/lib/Lower/Mangler.cpp flang/lib/Optimizer/Builder/IntrinsicCall.cpp flang/lib/Optimizer/CodeGen/TypeConverter.cpp flang/lib/Optimizer/Dialect/FIRType.cpp flang/lib/Optimizer/Transforms/AddDebugInfo.cpp flang/lib/Optimizer/Transforms/AssumedRankOpConversion.cpp flang/lib/Optimizer/Transforms/CUFDeviceGlobal.cpp flang/lib/Optimizer/Transforms/CUFGPUToLLVMConversion.cpp flang/lib/Optimizer/Transforms/CUFOpConversion.cpp flang/lib/Optimizer/Transforms/ExternalNameConversion.cpp flang/lib/Optimizer/Transforms/LoopVersioning.cpp flang/lib/Optimizer/Transforms/SimplifyIntrinsics.cpp flang/lib/Optimizer/Transforms/StackReclaim.cpp flang/lib/Optimizer/Transforms/VScaleAttr.cpp flang/lib/Parser/basic-parsers.h flang/lib/Parser/parse-tree.cpp flang/lib/Parser/prescan.h flang/lib/Parser/unparse.cpp flang/lib/Semantics/assignment.h flang/lib/Semantics/check-case.cpp flang/lib/Semantics/check-coarray.cpp flang/lib/Semantics/check-cuda.cpp flang/lib/Semantics/check-data.h flang/lib/Semantics/check-do-forall.cpp flang/lib/Semantics/check-return.cpp flang/lib/Semantics/check-select-rank.cpp flang/lib/Semantics/check-select-type.cpp flang/lib/Semantics/check-stop.cpp flang/lib/Semantics/data-to-inits.h flang/lib/Semantics/expression.cpp flang/lib/Semantics/pointer-assignment.cpp flang/lib/Semantics/resolve-labels.cpp flang/lib/Semantics/resolve-names-utils.cpp flang/lib/Semantics/resolve-names.cpp flang/lib/Semantics/rewrite-parse-tree.cpp flang/lib/Semantics/semantics.cpp flang/lib/Semantics/tools.cpp flang/lib/Support/Timing.cpp flang/runtime/CUDA/allocator.cpp flang/runtime/ISO_Fortran_binding.cpp flang/runtime/ISO_Fortran_util.h flang/runtime/allocatable.cpp flang/runtime/stat.h flang/runtime/temporary-stack.cpp flang/tools/bbc/bbc.cpp flang/tools/f18-parse-demo/f18-parse-demo.cpp flang/unittests/Evaluate/ISO-Fortran-binding.cpp flang/unittests/Runtime/CUDA/Allocatable.cpp flang/unittests/Runtime/CUDA/AllocatorCUF.cpp flang/unittests/Runtime/CUDA/Memory.cpp flang/unittests/Runtime/TemporaryStack.cpp flang/include/flang/Common/ISO_Fortran_binding_wrapper.h flang/include/flang/Support/Fortran-features.h flang/include/flang/Support/Fortran.h flang/include/flang/Support/LangOptions.h flang/include/flang/Support/MathOptionsBase.h flang/include/flang/Support/OpenMP-features.h flang/include/flang/Support/Version.h flang/include/flang/Support/default-kinds.h flang/include/flang/Support/indirection.h flang/include/flang/Support/interval.h flang/include/flang/Support/reference-counted.h flang/include/flang/Support/reference.h flang/include/flang/Support/static-multimap-view.h flang/include/flang/Support/template.h flang/include/flang/Support/unwrap.h flang/lib/Support/Fortran-features.cpp flang/lib/Support/Fortran.cpp flang/lib/Support/LangOptions.cpp flang/lib/Support/OpenMP-utils.cpp flang/lib/Support/Version.cpp flang/lib/Support/default-kinds.cpp flang/lib/Support/idioms.cpp
View the diff from clang-format here.
diff --git a/flang/include/flang/Frontend/CompilerInvocation.h b/flang/include/flang/Frontend/CompilerInvocation.h
index b3b7297f6f..9e6724be33 100644
--- a/flang/include/flang/Frontend/CompilerInvocation.h
+++ b/flang/include/flang/Frontend/CompilerInvocation.h
@@ -20,8 +20,8 @@
 #include "flang/Lower/LoweringOptions.h"
 #include "flang/Parser/parsing.h"
 #include "flang/Semantics/semantics.h"
-#include "mlir/Support/Timing.h"
 #include "flang/Support/LangOptions.h"
+#include "mlir/Support/Timing.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "llvm/Option/ArgList.h"

@Meinersbur Meinersbur force-pushed the users/meinersbur/flang_runtime_FortranSupport branch from 2023940 to 69f7bd6 Compare January 8, 2025 10:10
@Meinersbur Meinersbur marked this pull request as ready for review January 10, 2025 13:18
Copy link
Contributor

@jhuber6 jhuber6 left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me

@klausler
Copy link
Contributor

There's an implicit assumption here that either future runtime work won't occur, or won't need anything from Common that you're moving away into the new compiler-only directory.

Is this change required for your work that restructures the runtime builds?

@Meinersbur
Copy link
Member Author

Meinersbur commented Jan 11, 2025

Source compiled for the runtime has different requirements than the source compiled into the compiler. Any source that is not not used in the runtime yet will probably be incompatible with the runtime requirements. Even if it does, there is no reason to enforce runtime requirements to those sources that are not even used in the runtime.

Additional requirements include:

  • Must not cause a link-time dependency on the C++ standard library with any supported toolchain
  • Must be compilable as device code with nvcc/clang
  • Must not use #include <optional> or #include <variant>, but the replacements #include "optional.h" and #include "variant.h"
  • Must use the annotation macros from api-attrs.h
  • If used to interact with the runtime, must be target-independent (e.g. no sizeof(Descriptor))
  • ...

These requirements should really be formalized if we want to keep it maintanable. The first step is to separate the sources for which these requirements apply from the sources to which they do not, done in this PR. Mistakes not complying to these requirements already happened, see #110244, #112188, and https://github.com/llvm/llvm-project/pull/122069/files#diff-849ce0fa56b431a9789f5edb13f3ef46386024e6c4344529326856c39501323dR27.

Currently all of the runtime-included files are header-only1, but there are any in the future2, it would require PARTIAL_SOURCES_INTENDED, i.e. such thing is actively discouraged by the LLVM coding policy. PARTIAL_SOURCES_INTENDED currently can only check translation units, and FortranCommon has none needed for the runtime right now, but I don't think to stay like that forever2.

If in the future one source file is handy for the runtime as well, it can be moved into Common, but I doubt it would be the entirety of its defintions but only some that are actually needed. If that happens, the patch would need to ensure that the runtime requirements are met. This applies to any file in any directory btw. IMHO it is a poor argument to retain bad coding practices.

It is needed in the sense that without this separation, I would never have understood what defintions are actually part of the runtime. Why Fortran-features.h uses #include <optional> but fast-int-set.h must use #include "optional.h". Please don't make anyone else go through this.

Footnotes

  1. With the exception of FortranDecimal and Fortran.h which was resolved in [Flang][NFC] Split common headers to reduce dependencies. #110244.

  2. I suspect some of these are header-only only because they are included into the runtime. Some definitions and tables from e.g. format.h and leading-zero-bit-count.h are usually considered way too large to be inline defintions. I would have considered FortranDecimal to be typical common code. I don't think we want to introduce yet another library for every common utility just because FortranCommon is not structured well. 2

Copy link
Contributor

@jplehr jplehr left a comment

Choose a reason for hiding this comment

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

I did not find issues when running this through a couple of configurations I typically use.

@Meinersbur
Copy link
Member Author

FortranSupport has just been introduced in #122894.

@jeanPerier
Copy link
Contributor

@Meinersbur, is it possible to move this patch outside of the chain for the runtime change?

While your patch has a rational and relates to the runtime, I do not think it is needed functionally for the runtime cmake/project change and could be discussed later so that you can progress on the core change.

@klausler's point is that the Common library has some functional consistency, mainly about core Fortran definitions, and that breaking it up based on today's runtime usage of it is a bit artificial (some features from it that are not used today in the runtime may very well be already usable and may be used tomorrow).

@Meinersbur
Copy link
Member Author

Moving this PR out of the chain causes merge conflicts further down, making maintaining consistency of the series even more difficult. At least the change std::optional to optional.h is needed or I cannot compile it with nvcc (I am not sure how you do). I understand that with submitting patches I accept some responsibility to maintain it. I don't want to do so if it is not in a maintainable state.

I will put this PR as first into the chain, so at least it can be applied independently.

breaking it up based on today's runtime usage of it is a bit artificial (some features from it that are not used today in the runtime may very well be already usable and may be used tomorrow).

The current FortranCommon is already artificial presumably because it has grown organically: Everything that does not belong to some other library. Some of it (e.g. genEntryBlock from OpenMP-utils.h, getFlangRepositoryString() from Version.h) should conceptually never be used by the runtime. Splitting it up by usages actually makes it less artificial.

Any code that is currently not be used in the runtime should be assumed to not work with the runtime. E.g. because it causes an additional link dependency to FortranCommon.a/so or libc++, cannot be compiled with nvcc, or requires annotation for offloading. Similarly, code that is not being tested can be assumed to not work or after changes to the future. That is, additional work to make it usable within the runtime is required anyways.

Every other code, including those in other libraries, might also be eventually be useful for the runtime (e.g. if we are to include a JIT). That is not a reason to preemptively make all of them a dependency of the runtime.

Partial use of libraries is called "erroneous configuration" in LLVM, see ebc3302

Sorry, I cannot follow the argument.

@Meinersbur Meinersbur force-pushed the users/meinersbur/flang_runtime_FortranSupport branch from 32e5b84 to 72e3c5d Compare January 24, 2025 18:42
@Meinersbur Meinersbur merged commit 72e3c5d into users/meinersbur/flang_runtime_FortranDecimal Jan 24, 2025
2 of 4 checks passed
@Meinersbur Meinersbur force-pushed the users/meinersbur/flang_runtime_FortranDecimal branch from f2abcde to 2e50a1f Compare January 24, 2025 18:42
@Meinersbur Meinersbur deleted the users/meinersbur/flang_runtime_FortranSupport branch January 24, 2025 18:42
@Meinersbur Meinersbur restored the users/meinersbur/flang_runtime_FortranSupport branch January 24, 2025 18:44
@Meinersbur
Copy link
Member Author

Meinersbur commented Jan 24, 2025

GitHub interpreted pushing to the target branch (NOT main) of the patch series as "merging". There seems to be no way te re-open this PR, I will create a new one.

New PR: #124416

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants