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][Parser] Add missing dependencies to CMakeLists.txt #77483

Merged
merged 3 commits into from
Jan 11, 2024

Conversation

kparzysz
Copy link
Contributor

@kparzysz kparzysz commented Jan 9, 2024

Two additional component libraries: FrontendOpenACC and FrontendOpenMP.

Two additional component libraries: FrontendOpenACC and FrontendOpenMP.
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels Jan 9, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 9, 2024

@llvm/pr-subscribers-flang-parser

@llvm/pr-subscribers-flang-fir-hlfir

Author: Krzysztof Parzyszek (kparzysz)

Changes

Two additional component libraries: FrontendOpenACC and FrontendOpenMP.


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

1 Files Affected:

  • (modified) flang/lib/Lower/CMakeLists.txt (+2)
diff --git a/flang/lib/Lower/CMakeLists.txt b/flang/lib/Lower/CMakeLists.txt
index b13d415e02f1d9..8183d4280c2597 100644
--- a/flang/lib/Lower/CMakeLists.txt
+++ b/flang/lib/Lower/CMakeLists.txt
@@ -56,5 +56,7 @@ add_flang_library(FortranLower
   MLIRSCFToControlFlow
 
   LINK_COMPONENTS
+  FrontendOpenACC
+  FrontendOpenMP
   Support
 )

@kparzysz
Copy link
Contributor Author

This is needed for getting directive names for both OpenMP and OpenACC when using DumpTree from the AST dumper.

@kiranchandramohan
Copy link
Contributor

This is needed for getting directive names for both OpenMP and OpenACC when using DumpTree from the AST dumper.

I see that FortranParser has a link_component entry for FrontendOpenACC, is it enough to add an entry for FrontendOpenMP there?

add_flang_library(FortranParser
  Fortran-parsers.cpp
  ...

  LINK_LIBS
  FortranCommon

  LINK_COMPONENTS
  Support
  FrontendOpenACC

  DEPENDS
  omp_gen
  acc_gen
)

@clementval
Copy link
Contributor

This is needed for getting directive names for both OpenMP and OpenACC when using DumpTree from the AST dumper.

DumpTree is not part of Lowering libs. I guess @kiranchandramohan suggestion should work for DumpTree.

@kparzysz
Copy link
Contributor Author

Sounds good. Will update the PR later today.

@kparzysz kparzysz changed the title [Flang][Lower] Add missing dependencies to CMakeLists.txt [Flang][Parser] Add missing dependencies to CMakeLists.txt Jan 10, 2024
Copy link
Contributor

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

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

LGTM.

@kparzysz kparzysz merged commit cc53ec8 into llvm:main Jan 11, 2024
5 checks passed
@kparzysz kparzysz deleted the users/kparzysz/missing-deps branch January 11, 2024 11:39
@kparzysz
Copy link
Contributor Author

Bad news, this didn't work. I put DumpTree in OpenMP.cpp, and I'm getting

/usr/bin/ld: CMakeFiles/obj.FortranLower.dir/OpenMP.cpp.o: in function `std::enable_if<((!(is_class_v<llvm::acc::Directive>))||(is_same_v<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, llvm::acc::Directive>))||(is_same_v<Fortran::parser::CharBlock, llvm::acc::Directive>), void>::type Fortran::parser::Walk<llvm::acc::Directive, Fortran::parser::ParseTreeDumper>(llvm::acc::Directive const&, Fortran::parser::ParseTreeDumper&)':
OpenMP.cpp:(.text._ZN7Fortran6parser4WalkIN4llvm3acc9DirectiveENS0_15ParseTreeDumperEEENSt9enable_ifIXoooont10is_class_vIT_E9is_same_vINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEES7_E9is_same_vINS0_9CharBlockES7_EEvE4typeERKS7_RT0_[_ZN7Fortran6parser4WalkIN4llvm3acc9DirectiveENS0_15ParseTreeDumperEEENSt9enable_ifIXoooont10is_class_vIT_E9is_same_vINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEES7_E9is_same_vINS0_9CharBlockES7_EEvE4typeERKS7_RT0_]+0x146): undefined reference to `llvm::acc::getOpenACCDirectiveName(llvm::acc::Directive)'
/usr/bin/ld: CMakeFiles/obj.FortranLower.dir/OpenMP.cpp.o: in function `std::enable_if<WrapperTrait<Fortran::parser::AccDefaultClause>, void>::type Fortran::parser::Walk<Fortran::parser::AccDefaultClause, Fortran::parser::ParseTreeDumper>(Fortran::parser::AccDefaultClause const&, Fortran::parser::ParseTreeDumper&)':
OpenMP.cpp:(.text._ZN7Fortran6parser4WalkINS0_16AccDefaultClauseENS0_15ParseTreeDumperEEENSt9enable_ifIX12WrapperTraitIT_EEvE4typeERKS5_RT0_[_ZN7Fortran6parser4WalkINS0_16AccDefaultClauseENS0_15ParseTreeDumperEEENSt9enable_ifIX12WrapperTraitIT_EEvE4typeERKS5_RT0_]+0x27c): undefined reference to `llvm::acc::getOpenACCDefaultValueName(llvm::acc::DefaultValue)'
collect2: error: ld returned 1 exit status```

@clementval
Copy link
Contributor

Why would you put DumpTree in lowering? For local debugging?

@kparzysz
Copy link
Contributor Author

Why would you put DumpTree in lowering? For local debugging?

Basically, yes. I can add these libraries when I need them, so this is not a big deal. I should probably revert this commit though, since it doesn't accomplish anything.

@kparzysz
Copy link
Contributor Author

Although... If I don't need to link any extra libraries do get DumpTree, I'd expect it to just work.

@clementval
Copy link
Contributor

Well DumpTree is not really meant to be called from lowering so I would not add the library unless there is an upstream usage of it.

@clementval
Copy link
Contributor

FrontendOpenACC was added a while back to fix shared library dependencies https://reviews.llvm.org/D83938. I'm not sure why FrontendOpenMP was not added at the same time or if both are really needed.

kparzysz added a commit that referenced this pull request Jan 11, 2024
…77483)"

This reverts commit cc53ec8.

This commit hasn't accomplished anything. The original issue was that
`DumpTree`, when called from lowering, caused linker errors due to some
directive-naming functions being absent. Adding FrontendOpenMP to the
parser library didn't fix that problem, and according to the notes in
PR #77483, calling `DumpTree` from lowering isn't really supported.
@kparzysz
Copy link
Contributor Author

When I looked at the "needed" libraries for libFortranParser.so, it still only showed the OpenACC one, even with the OpenMP library in CMakeLists.txt. I don't know why.

@clementval
Copy link
Contributor

There are some usage of llvm::omp::getOpenMPDirectiveName in OpenMP.cpp so DumpTree should work for you. Do you build shared libraries or out of tree?

@kparzysz
Copy link
Contributor Author

I build shared libraries. Looking again at the error messages---it seems like it's this symbol that's missing: llvm::acc::getOpenACCDirectiveName(llvm::acc::Directive).

justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
Add FrontendOpenMP as an additional component library dependency.
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
…lvm#77483)"

This reverts commit cc53ec8.

This commit hasn't accomplished anything. The original issue was that
`DumpTree`, when called from lowering, caused linker errors due to some
directive-naming functions being absent. Adding FrontendOpenMP to the
parser library didn't fix that problem, and according to the notes in
PR llvm#77483, calling `DumpTree` from lowering isn't really supported.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:fir-hlfir flang:parser flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants