RFC: FIR Merge - PR1: Add PFTBuilder structure to help lowering the parse-tree #959

jeanPerier · 2020-01-28T21:38:39Z

This PR contains the directory structure and .clang-format for FIR and the first piece of code: the ASTBuilder.

The ASTBuilder structure is a transient data structure that
is meant to be built from the parse tree just before lowering to
FIR and that will be deleted just afterwards. It is not meant to perform
optimization analysis and transformations. It only provides temporary
information, such as label target information or parse tree parent nodes,
that is meant to be used to lower the parse tree structure into
FIR operations.
A pretty printer is available to visualize this data structure.

This code depends on LLVM LLVMSupport library but does not require MLIR (it can build without changes to the current main CMakeLists.txt).

Note that the commits does not reflect an actual work log, it is a feature based split of the
changes done in the FIR experimental branch. The related work log can be found in the commits between: 8c320e3 and 9b9ea05.

include/flang/lower/ASTBuilder.h

lib/lower/ASTBuilder.cpp

.clang-format

lib/lower/ASTBuilder.cpp

kiranchandramohan

Thanks for breaking the FIR merge into a series a PRs.

Can you add a statement to the commit message on why having the AST is useful for constructing the CFG rather than directly constructing it?

Can you add a few tests to demonstrate and test the functionality? It seems you have a pretty-printer/dumpAST function which can be used for this purpose.

include/flang/lower/ASTBuilder.h

lib/lower/ASTBuilder.cpp

include/flang/lower/ASTBuilder.h

Review: flang-compiler#959 Fixes file names in LLVM copyright.

include/flang/lower/ASTBuilder.h

DavidTruby · 2020-01-29T15:00:28Z

include/flang/lower/ASTBuilder.h

+  std::list<FunctionStatement> funStmts;  // begin/end pair
+  std::list<Evaluation> evals;            // statements
+  std::list<FunctionLikeUnit> funcs;      // internal procedures


See previous comment about std::list. If the reason for using std::list is that these need push and pop from both the back and front then std::deque may be a better choice. However in this PR I only see peek at the front and back and push at the back, in which case std::vector is probably more appropriate.

Regarding the std::list vs std::vector, there is another a reason for actually using a list right now; the address of elements inside the lists are taken. See:

FunctionLikeUnit::evals elements address are taken here: https://github.com/jeanPerier/f18/blob/10e01a4e505fe6f874dffe4d5f08d2503a695576/lib/lower/ASTBuilder.cpp#L308

ModuleLikeUnit::funcs elements address are taken here: https://github.com/jeanPerier/f18/blob/10e01a4e505fe6f874dffe4d5f08d2503a695576/lib/lower/ASTBuilder.cpp#L323

FunctionLikeUnit::funcs elements are indirectly taken (ASTBuilder::funclist points to funcs, address of element of *funclist are taken) at:
https://github.com/jeanPerier/f18/blob/10e01a4e505fe6f874dffe4d5f08d2503a695576/lib/lower/ASTBuilder.cpp#L291

Program::units elements address here:
https://github.com/jeanPerier/f18/blob/10e01a4e505fe6f874dffe4d5f08d2503a695576/lib/lower/ASTBuilder.cpp#L323

Because std::vector element addresses may be invalidated after insertion, that would be a risky choice here.

If the addresses are taken and we might resize and invalidate those addresses, std::deque can be used instead of std::list as that has the same guarantee that addresses will not be invalidated after resizing.

std::list was used intentionally. The reason for that was to allow O(1) insertions anywhere in the structure to simplify lowering.

include/flang/lower/ASTBuilder.h

DavidTruby · 2020-01-29T15:13:21Z

lib/lower/ASTBuilder.cpp

+    for (const auto &control : stmt.controls) {
+      if (std::holds_alternative<Pa::ErrLabel>(control.u)) {
+        return true;
+      }
+    }


nit suggestion:

return std::any_of(std::begin(stmt.controls), std::end(stmt.controls), [](const auto& control) { return std::holds_alternative<Pa::ErrLabel>(control.u); });

Or even better because this happens multiple times, add:

auto isError = [](const auto& v) { return std::holds_alternative<Pa::ErrLabel>(v.u); };

before the if constexpr chain and then use

return std::any_of(std::begin(stmt.controls), std::end(stmt.controls), isError);

in each if constexpr branch.

Thanks for the suggestion, it seemed to me a good idea so I did do that.
@klausler, what did you down-voted that ?

The range-based for idiom is more clear than the cluttered call to the iterator-based "algorithm".

I don't really agree that the range-for is more clear, I think in the any_of case it's really easy to see that you're trying to do a boolean check on the entire vector. However as I said in my suggestion I don't feel particularly strongly about it, I just thought this read a little better.

include/flang/lower/ASTBuilder.h

lib/lower/ASTBuilder.cpp

lib/lower/PFTBuilder.cpp

include/flang/lower/PFTBuilder.h

jeanPerier · 2020-02-12T16:08:03Z

Regarding @kiranchandramohan comments:

Can you add a few tests to demonstrate and test the functionality? It seems you have a pretty-printer/dumpAST function which can be used for this purpose.

I have added LIT tests to demonstrate that the Pre FIR tree captures the program structure as intended. I added an option to f18 driver to dump the PFT. Beware, this will bring the first real link-time dependency to LLVM libraries.

tskeith · 2020-02-12T19:21:05Z

Compiling f18.cpp I'm getting errors like this (clang 9 on macos):

In file included from ../../tools/f18/f18.cpp:14:
In file included from ../../include/flang/lower/PFTBuilder.h:14:
In file included from /Users/tsk/llvm/f18-llvm/llvm/include/llvm/Support/raw_ostream.h:16:
In file included from /Users/tsk/llvm/f18-llvm/llvm/include/llvm/ADT/SmallVector.h:17:
In file included from /Users/tsk/llvm/f18-llvm/llvm/include/llvm/Support/AlignOf.h:16:
/Users/tsk/llvm/f18-llvm/llvm/include/llvm/Support/Compiler.h:571:49: error: unused parameter 'Size' [-Werror,-Wunused-parameter]
inline void deallocate_buffer(void *Ptr, size_t Size, size_t Alignment) {
                                                ^

This is deallocate_buffer:

inline void deallocate_buffer(void *Ptr, size_t Size, size_t Alignment) {
  ::operator delete(Ptr
#ifdef __cpp_sized_deallocation
                    ,
                    Size
#endif
#ifdef __cpp_aligned_new
                    ,
                    std::align_val_t(Alignment)
#endif
  );
}

So you may or may not get unused parameter errors depending on the #defines in effect.

lib/lower/CMakeLists.txt

tskeith · 2020-02-12T22:37:42Z

For what it's worth, I was able to build and pass the tests on macos using clang 9 and LLVM built with clang 9.

I wasn't able to get it to work with gcc: f18 built with gcc doesn't link against LLVM built with clang, and I wasn't able to build LLVM with gcc.

include/flang/lower/PFTBuilder.h

sscalpone · 2020-02-14T04:45:07Z

Perhaps change the title of the PR to match the new name.

The Pre-FIR Tree structure is a temporary data structure that is meant to be built from the parse tree just before lowering to FIR and that will be deleted just afterwards. It is not meant to perfrom optimization analysis and transformations. It only provides temporary information, such as label target information or parse tree parent nodes, that is meant to be used to lower the parse tree structure into FIR operations. A PFTBuilder class builds the Pre-Fir Tree from the parse-tree. A pretty printer is available to visualize this data structure. - Lit tests are added to: 1. that the PFT tree structure is as expected 2. that the PFT captures all intented nodes - Cmake changes: Prevent warnings inisde LLVM headers when compiling flang The issue is that some LLVM headers define functions where the usage of the parameters depend on environment ifdef. See for instance Size in: https://github.com/llvm/llvm-project/blob/5f940220bf9438e95ffa4a627ac1591be1e1ba6e/llvm/include/llvm/Support/Compiler.h#L574 Because flang is build with -Werror and -Wunused-parameter is default in clang, this may breaks build in some environments (like with clang9 on macos). A solution would be to add -Wno-unused-parameter to flang CmakLists.txt, but it is wished to keep this warning on flang sources for quality purposes. Fixing LLVM headers is not an easy task and `[[maybe_unused]]` is C++17 and cannot be used yet in LLVM headers. Hence, this fix simply silence warnings coming from LLVM headers by telling CMake they are to be considered as if they were system headers. - drone.io changes: remove llvm 6.0 from clang config in drone.io llvm-dev resolved to llvm-6.0 in clang builds on drone.io. llvm 6.0 package is linked with a conflicting c++ standard library with the one used for flang. This caused link time failure when building clang. Note: This commit does not reflect an actual work log, it is a feature based split of the changes done in the FIR experimental branch. The related work log can be found in the commits between: 864898c and 137c23d Other changes come from flang-compiler#959 review.

kiranchandramohan

Thanks @jeanPerier for making available the tests.

Is there a test for CGJump?

tools/f18/f18.cpp

test-lit/lower/pre-fir-tree01.f90

jeanPerier · 2020-02-14T12:03:20Z

Thanks @jeanPerier for making available the tests.

Is there a test for CGJump?

No because the code that is intended to introduce them comes in a later patch (the PR with the bridge mentioned in an earlier thread). By the way, CGjump may be deleted in the FIR development fork, so it may be removed from here in the later PR with bridge. And if it is still there, related tests will come with this PR.

tskeith · 2020-02-14T19:18:46Z

For what it's worth, I was able to build and pass the tests on macos using clang 9 and LLVM built with clang 9.

I wasn't able to get it to work with gcc: f18 built with gcc doesn't link against LLVM built with clang, and I wasn't able to build LLVM with gcc.

I've found that I can use the same build of LLVM (compiled with clang) for both gcc and clang builds of f18 by adding this to the gcc cmake command:
-DCMAKE_EXE_LINKER_FLAGS="-L$CLANG/lib -lc++"

Note: This commit does not reflect an actual work log, it is a feature based split of the changes done in the FIR experimental branch. The related work log can be found in the commits between: schweitzpgi@8c320e3 and: schweitzpgi@9b9ea05

The Pre-FIR Tree structure is a transient data structure that is meant to be built from the parse tree just before lowering to FIR and that will be deleted just afterwards. It is not meant to perfrom optimization analysis and transformations. It only provides temporary information, such as label target information or parse tree parent nodes, that is meant to be used to lower the parse tree structure into FIR operations. A PFTBuilder class builds the Pre-Fir Tree from the parse-tree. A pretty printer is available to visualize this data structure. - Lit tests are added to: 1. that the PFT tree structure is as expected 2. that the PFT captures all intented nodes - Cmake changes: Prevent warnings inisde LLVM headers when compiling flang The issue is that some LLVM headers define functions where the usage of the parameters depend on environment ifdef. See for instance Size in: https://github.com/llvm/llvm-project/blob/5f940220bf9438e95ffa4a627ac1591be1e1ba6e/llvm/include/llvm/Support/Compiler.h#L574 Because flang is build with -Werror and -Wunused-parameter is default in clang, this may breaks build in some environments (like with clang9 on macos). A solution would be to add -Wno-unused-parameter to flang CmakLists.txt, but it is wished to keep this warning on flang sources for quality purposes. Fixing LLVM headers is not an easy task and `[[maybe_unused]]` is C++17 and cannot be used yet in LLVM headers. Hence, this fix simply silence warnings coming from LLVM headers by telling CMake they are to be considered as if they were system headers. - drone.io changes: remove llvm 6.0 from clang config in drone.io and link flang with libstdc++ instead of libc++ llvm-dev resolved to llvm-6.0 in clang builds on drone.io. llvm 6.0 too old. LLVM packages are linked with libstdc++ standard library whereas libc++ was used for flang. This caused link time failure when building clang. Change frone.io to build flang with libc++. Note: This commit does not reflect an actual work log, it is a feature based split of the changes done in the FIR experimental branch. The related work log can be found in the commits between: 864898c and 137c23d Other changes come from flang-compiler#959 review.

Note: This commit does not reflect an actual work log, it is a feature based split of the changes done in the FIR experimental branch. The related work log can be found in the commits between: schweitzpgi/obsolete-f18@8c320e3 and: schweitzpgi/obsolete-f18@9b9ea05 Original-commit: flang-compiler/f18@00d8d51 Reviewed-on: flang-compiler/f18#959 Tree-same-pre-rewrite: false

The Pre-FIR Tree structure is a transient data structure that is meant to be built from the parse tree just before lowering to FIR and that will be deleted just afterwards. It is not meant to perfrom optimization analysis and transformations. It only provides temporary information, such as label target information or parse tree parent nodes, that is meant to be used to lower the parse tree structure into FIR operations. A PFTBuilder class builds the Pre-Fir Tree from the parse-tree. A pretty printer is available to visualize this data structure. - Lit tests are added to: 1. that the PFT tree structure is as expected 2. that the PFT captures all intented nodes - Cmake changes: Prevent warnings inisde LLVM headers when compiling flang The issue is that some LLVM headers define functions where the usage of the parameters depend on environment ifdef. See for instance Size in: https://github.com/llvm/llvm-project/blob/5f940220bf9438e95ffa4a627ac1591be1e1ba6e/llvm/include/llvm/Support/Compiler.h#L574 Because flang is build with -Werror and -Wunused-parameter is default in clang, this may breaks build in some environments (like with clang9 on macos). A solution would be to add -Wno-unused-parameter to flang CmakLists.txt, but it is wished to keep this warning on flang sources for quality purposes. Fixing LLVM headers is not an easy task and `[[maybe_unused]]` is C++17 and cannot be used yet in LLVM headers. Hence, this fix simply silence warnings coming from LLVM headers by telling CMake they are to be considered as if they were system headers. - drone.io changes: remove llvm 6.0 from clang config in drone.io and link flang with libstdc++ instead of libc++ llvm-dev resolved to llvm-6.0 in clang builds on drone.io. llvm 6.0 too old. LLVM packages are linked with libstdc++ standard library whereas libc++ was used for flang. This caused link time failure when building clang. Change frone.io to build flang with libc++. Note: This commit does not reflect an actual work log, it is a feature based split of the changes done in the FIR experimental branch. The related work log can be found in the commits between: 864898cbe509d032abfe1172ec367dbd3dd92bc1 and 137c23da9c64cf90584cf81fd646053a69e91f63 Other changes come from flang-compiler/f18#959 review. Original-commit: flang-compiler/f18@edb0943 Reviewed-on: flang-compiler/f18#959

…-mono-split RFC: FIR Merge - PR1: Add PFTBuilder structure to help lowering the parse-tree Original-commit: flang-compiler/f18@f28afa7 Reviewed-on: flang-compiler/f18#959

Note: This commit does not reflect an actual work log, it is a feature based split of the changes done in the FIR experimental branch. The related work log can be found in the commits between: schweitzpgi/obsolete-f18@8c320e3 and: schweitzpgi/obsolete-f18@9b9ea05 Original-commit: flang-compiler/f18@00d8d51 Reviewed-on: flang-compiler/f18#959 Tree-same-pre-rewrite: false

The Pre-FIR Tree structure is a transient data structure that is meant to be built from the parse tree just before lowering to FIR and that will be deleted just afterwards. It is not meant to perfrom optimization analysis and transformations. It only provides temporary information, such as label target information or parse tree parent nodes, that is meant to be used to lower the parse tree structure into FIR operations. A PFTBuilder class builds the Pre-Fir Tree from the parse-tree. A pretty printer is available to visualize this data structure. - Lit tests are added to: 1. that the PFT tree structure is as expected 2. that the PFT captures all intented nodes - Cmake changes: Prevent warnings inisde LLVM headers when compiling flang The issue is that some LLVM headers define functions where the usage of the parameters depend on environment ifdef. See for instance Size in: https://github.com/llvm/llvm-project/blob/5f940220bf9438e95ffa4a627ac1591be1e1ba6e/llvm/include/llvm/Support/Compiler.h#L574 Because flang is build with -Werror and -Wunused-parameter is default in clang, this may breaks build in some environments (like with clang9 on macos). A solution would be to add -Wno-unused-parameter to flang CmakLists.txt, but it is wished to keep this warning on flang sources for quality purposes. Fixing LLVM headers is not an easy task and `[[maybe_unused]]` is C++17 and cannot be used yet in LLVM headers. Hence, this fix simply silence warnings coming from LLVM headers by telling CMake they are to be considered as if they were system headers. - drone.io changes: remove llvm 6.0 from clang config in drone.io and link flang with libstdc++ instead of libc++ llvm-dev resolved to llvm-6.0 in clang builds on drone.io. llvm 6.0 too old. LLVM packages are linked with libstdc++ standard library whereas libc++ was used for flang. This caused link time failure when building clang. Change frone.io to build flang with libc++. Note: This commit does not reflect an actual work log, it is a feature based split of the changes done in the FIR experimental branch. The related work log can be found in the commits between: 864898cbe509d032abfe1172ec367dbd3dd92bc1 and 137c23da9c64cf90584cf81fd646053a69e91f63 Other changes come from flang-compiler/f18#959 review. Original-commit: flang-compiler/f18@edb0943 Reviewed-on: flang-compiler/f18#959

klausler reviewed Jan 28, 2020

View reviewed changes

include/flang/lower/ASTBuilder.h Outdated Show resolved Hide resolved

klausler reviewed Jan 28, 2020

View reviewed changes

lib/lower/ASTBuilder.cpp Outdated Show resolved Hide resolved

tskeith reviewed Jan 28, 2020

View reviewed changes

.clang-format Outdated Show resolved Hide resolved

tskeith reviewed Jan 28, 2020

View reviewed changes

lib/lower/ASTBuilder.cpp Outdated Show resolved Hide resolved

tskeith reviewed Jan 28, 2020

View reviewed changes

lib/lower/ASTBuilder.cpp Outdated Show resolved Hide resolved

tskeith reviewed Jan 28, 2020

View reviewed changes

lib/lower/ASTBuilder.cpp Outdated Show resolved Hide resolved

kiranchandramohan reviewed Jan 29, 2020

View reviewed changes

jeanPerier pushed a commit to jeanPerier/f18 that referenced this pull request Jan 29, 2020

review comments

5c3afc0

Review: flang-compiler#959 Fixes file names in LLVM copyright.

jeanPerier mentioned this pull request Jan 29, 2020

Address PR959 review comments schweitzpgi/obsolete-f18#43

Closed

tskeith reviewed Jan 29, 2020

View reviewed changes

include/flang/lower/ASTBuilder.h Outdated Show resolved Hide resolved

tskeith reviewed Jan 29, 2020

View reviewed changes

include/flang/lower/ASTBuilder.h Outdated Show resolved Hide resolved

DavidTruby suggested changes Jan 29, 2020

View reviewed changes

jeanPerier mentioned this pull request Jan 30, 2020

Bump clang-format version and remove some differences from LLVM #945

Merged

klausler suggested changes Jan 30, 2020

View reviewed changes

kiranchandramohan reviewed Jan 31, 2020

View reviewed changes

lib/lower/ASTBuilder.cpp Outdated Show resolved Hide resolved

jeanPerier mentioned this pull request Feb 4, 2020

Fix template step limit issue with clang #968

Merged

klausler approved these changes Feb 7, 2020

View reviewed changes

kiranchandramohan reviewed Feb 10, 2020

View reviewed changes

lib/lower/PFTBuilder.cpp Outdated Show resolved Hide resolved

lib/lower/PFTBuilder.cpp Show resolved Hide resolved

lib/lower/PFTBuilder.cpp Outdated Show resolved Hide resolved

tskeith reviewed Feb 10, 2020

View reviewed changes

include/flang/lower/PFTBuilder.h Outdated Show resolved Hide resolved

tskeith reviewed Feb 10, 2020

View reviewed changes

include/flang/lower/PFTBuilder.h Show resolved Hide resolved

tskeith reviewed Feb 12, 2020

View reviewed changes

lib/lower/CMakeLists.txt Outdated Show resolved Hide resolved

tskeith reviewed Feb 13, 2020

View reviewed changes

include/flang/lower/PFTBuilder.h Show resolved Hide resolved

tskeith reviewed Feb 13, 2020

View reviewed changes

include/flang/lower/PFTBuilder.h Outdated Show resolved Hide resolved

jeanPerier changed the title ~~RFC: FIR Merge - PR1: Add ASTBuilder structure to help lowering the parse-tree~~ RFC: FIR Merge - PR1: Add PFTBuilder structure to help lowering the parse-tree Feb 14, 2020

kiranchandramohan reviewed Feb 14, 2020

View reviewed changes

tools/f18/f18.cpp Outdated Show resolved Hide resolved

test-lit/lower/pre-fir-tree01.f90 Show resolved Hide resolved

jeanPerier added 2 commits February 17, 2020 02:37

jeanPerier force-pushed the jpr-mono-split branch from ee469d6 to edb0943 Compare February 17, 2020 15:48

jeanPerier merged commit f28afa7 into flang-compiler:master Feb 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: FIR Merge - PR1: Add PFTBuilder structure to help lowering the parse-tree #959

RFC: FIR Merge - PR1: Add PFTBuilder structure to help lowering the parse-tree #959

jeanPerier commented Jan 28, 2020

kiranchandramohan left a comment

DavidTruby Jan 29, 2020

jeanPerier Jan 30, 2020

DavidTruby Jan 30, 2020

schweitzpgi Jan 30, 2020

DavidTruby Jan 29, 2020

DavidTruby Jan 29, 2020 •

edited

jeanPerier Jan 30, 2020

klausler Jan 30, 2020

DavidTruby Feb 13, 2020

jeanPerier commented Feb 12, 2020

tskeith commented Feb 12, 2020

tskeith commented Feb 12, 2020

sscalpone commented Feb 14, 2020

kiranchandramohan left a comment

jeanPerier commented Feb 14, 2020

tskeith commented Feb 14, 2020

RFC: FIR Merge - PR1: Add PFTBuilder structure to help lowering the parse-tree #959

RFC: FIR Merge - PR1: Add PFTBuilder structure to help lowering the parse-tree #959

Conversation

jeanPerier commented Jan 28, 2020

kiranchandramohan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidTruby Jan 29, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanPerier commented Feb 12, 2020

tskeith commented Feb 12, 2020

tskeith commented Feb 12, 2020

sscalpone commented Feb 14, 2020

kiranchandramohan left a comment

Choose a reason for hiding this comment

jeanPerier commented Feb 14, 2020

tskeith commented Feb 14, 2020

DavidTruby Jan 29, 2020 •

edited