[Flang] fix dependency issues after D78215

[clementval]

Summary

Patch D78215 changes various dependencies in the CMakeLists.txt. This
results in error while compiling.

[  3%] Building CXX object tools/flang/lib/Parser/CMakeFiles/obj.FortranParser.dir/char-buffer.cpp.o
In file included from /home/4vn/versioning/llp/mlir/include/mlir/IR/Function.h:20:0,
                 from /home/4vn/versioning/llp/mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h:18,
                 from /home/4vn/versioning/llp/flang/lib/Optimizer/Support/KindMapping.cpp:10:
/home/4vn/versioning/llp/mlir/include/mlir/Interfaces/CallInterfaces.h:27:10: fatal error: mlir/Interfaces/CallInterfaces.h.inc: No such file or directory
 #include "mlir/Interfaces/CallInterfaces.h.inc"
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [tools/flang/lib/Optimizer/Support/CMakeFiles/obj.FIRSupport.dir/KindMapping.cpp.o] Error 1
make[1]: *** [tools/flang/lib/Optimizer/Support/CMakeFiles/obj.FIRSupport.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....

[DavidTruby]

I didn't see these errors when testing that patch, but I did somewhat expect to; this patch still works for me so if it fixes a build error at your end I'm happy to approve.

[ChinouneMehdi]

flang/lib/Evaluate/CMakeLists.txt

32

You are reintroducing a cycle-dependency between FortranEvaluate and FortranSemantics, and in addition FortranEvaluate doesn't call any function from FortranSemantics.

flang/lib/Optimizer/Dialect/CMakeLists.txt

11

FortranSupport is already declared as a dependency, why!

flang/lib/Optimizer/Support/CMakeLists.txt

6–7

Same here!

[clementval]

flang/lib/Evaluate/CMakeLists.txt

32

On my system there is a linking problem without this.

flang/lib/Optimizer/Dialect/CMakeLists.txt

11

Compile time dependency and link time dependency are not the same.

flang/lib/Optimizer/Support/CMakeLists.txt

6–7

This is needed as this is a compile time dependency as well. Before your patch everything was compiling fine. But not after it. Those change are fixing the problem your introduced on the system I use at least.

[clementval]

In D78340#1988471, @DavidTruby wrote:

I didn't see these errors when testing that patch, but I did somewhat expect to; this patch still works for me so if it fixes a build error at your end I'm happy to approve.

Thanks, can you land it for me?

[ChinouneMehdi]

This comment was removed by ChinouneMehdi.

[ChinouneMehdi]

flang/lib/Evaluate/CMakeLists.txt

32

Could you remove this unrelated dependency.

flang/lib/Optimizer/Dialect/CMakeLists.txt

11

Your problem was occurring with FIRSupport, so why introducing this dependency to FIRDialect?

flang/lib/Optimizer/Support/CMakeLists.txt

6–7

The problem was because of a file that wasn't generated before building FIRSupport, so to fix the problem it suffices to introduce a dependency on it ( implicitly or explicitly).

[clementval]

In D78340#1988692, @ChinouneMehdi wrote:

I think this change will fix your problem: D78354
Just a question: Have you try to investigate where the problem is before making these changes?

Of course I did. Do you think I just randomly try something to fix it. I find your comment quite offensive, please try to think before asking this kind of question. Your patch for sure doesn't fix my problem with FIRSemantics.

flang/lib/Evaluate/CMakeLists.txt

32

I just mentioned that it is needed. If on your system it works this is not the case for everybody. You tied your patch with 3 compilers and on one system. The reality is much more complex.

flang/lib/Optimizer/Dialect/CMakeLists.txt

11

Obviously, I didn't attached the 5-6 errors occurring. The change I posted in this patch are needed at least on the system I used to compile Flang (Summit node).

[ChinouneMehdi]

In D78340#1988739, @clementval wrote:

In D78340#1988692, @ChinouneMehdi wrote:

I think this change will fix your problem: D78354
Just a question: Have you try to investigate where the problem is before making these changes?

Of course I did. Do you think I just randomly try something to fix it. I find your comment quite offensive, please try to think before asking this kind of question. Your patch for sure doesn't fix my problem with FIRSemantics.

Sorry, I didn't mean no offense.

[clementval]

In D78340#1988766, @ChinouneMehdi wrote:

In D78340#1988739, @clementval wrote:

In D78340#1988692, @ChinouneMehdi wrote:

I think this change will fix your problem: D78354
Just a question: Have you try to investigate where the problem is before making these changes?

Of course I did. Do you think I just randomly try something to fix it. I find your comment quite offensive, please try to think before asking this kind of question. Your patch for sure doesn't fix my problem with FIRSemantics.

Sorry, I didn't mean no offense.

All good

flang/lib/Optimizer/Support/CMakeLists.txt

6–7

I agree with this ... those file are generated by Tablegen so of course we can put those dependency here but it is not cleaner to depends on the dialects since they are ultimately generating those files and these are the dependencies one would see when looking at the header in FIRSupport and FIRDialect?

[stephenneuendorffer]

flang/lib/Optimizer/Dialect/CMakeLists.txt

11

This should not be necessary since FIRSupport is a library dependency.

flang/lib/Optimizer/Support/CMakeLists.txt

6–7

Cmake *should* generate dependencies to ensure that the tablegen files are generate before the dialect, even if the libraries are not specified as explicit dependencies. I suspect there is something else going on here. What version of cmake and what configuration are you using?

[clementval]

flang/lib/Optimizer/Support/CMakeLists.txt

6–7

Well unfortunately it does not and this fixes it. I use Cmake 3.15.2 with GNU 7.5 on a Power9 with 64 thread to compile.

[DavidTruby]

@ChinouneMehdi can you tell me which compiler and linker you're seeing issues with with this patch? I'd like to try and reproduce.

We seem to be in a situation where the previous patch fixed some builds and broke some others, and this does the reverse. Hopefully we can come to something that doesn't break either build!

flang/lib/Optimizer/Support/CMakeLists.txt

6–7

I'll try and reproduce this error as I didn't see it yesterday (I wasn't using gcc 7). Which linker were you using, the default (bfd)?

[DavidTruby]

flang/lib/Optimizer/Support/CMakeLists.txt

6–7

Interesting. I'm unable to build mlir at all with gcc 7, I simply get told that the host compiler doesn't support std::atomic. Perhaps this is specifically the case on my platform (AArch64)?
Are you building with LLVM_ENABLE_PROJECTS="mlir;flang"?

[clementval]

flang/lib/Optimizer/Dialect/CMakeLists.txt

11

Since FIRDialect need also the file generated by Tablegen, this dependency solves it. Of course, we can add the dialect dependency here as well.

[DavidTruby]

flang/lib/Optimizer/Support/CMakeLists.txt

6–7

Never mind; misconfiguration in my gcc 7 install...

[clementval]

Remove indirect dependency and use direct one.

[DavidTruby]

I can reproduce @clementval's issue, but only when using make and not when using ninja which seems very odd to me. I do also expect the circular dependency to be needed at the moment so I think this patch is correct on that point. Fixing the circular dependency needs work outside the build system as there is actually a circular dependency in the code.
I'm inclined to say that given we broke something that used to work, we should merge this patch to fix that even if it breaks things that the last patch fixed.

[clementval]

flang/lib/Optimizer/Dialect/CMakeLists.txt

11

@stephenneuendorffer I changed to add the dependency on dialect instead indirect via FIRSupport

[clementval]

In D78340#1989032, @DavidTruby wrote:

I can reproduce @clementval's issue, but only when using make and not when using ninja which seems very odd to me. I do also expect the circular dependency to be needed at the moment so I think this patch is correct on that point. Fixing the circular dependency needs work outside the build system as there is actually a circular dependency in the code.
I'm inclined to say that given we broke something that used to work, we should merge this patch to fix that even if it breaks things that the last patch fixed.

I'm using make since I do not have ninja on the system I used currently and cannot install it. This is probably why I see the problem.

[stephenneuendorffer]

FWIW, I think my expectations are correct, if you include the dialect libraries as a PUBLIC dependency. In your case, using LINK_LIBS results in target_link_libraries being called with all the libraries as INTERFACE libraries.
It's unclear why there is a distinction made here, since it seems logical that cmake handles PUBLIC and INTERFACE dependencies the same with respect to header files, but there must be some reason :)

[clementval]

@DavidTruby Can we at least reintroduce FortranSemantics since it is a know problem with make and was working before? For the other problem, I can live with it for the moment since I can build MLIR first and then Flang even though it will be better to have a fix for that as well.

[DavidTruby]

Yes, please merge. I am happy with the other change too, don't see why you can't merge that as I don't think it's breaking things for anyone?

[clementval]

In D78340#1994732, @DavidTruby wrote:

Yes, please merge. I am happy with the other change too, don't see why you can't merge that as I don't think it's breaking things for anyone?

@DavidTruby Can I ask you to land it? I do not have the rights.

[richard.barton.arm]

@clementval - you should ask for commit access. I think already you have shown yourself to be a serious contributor, you should have no problem being granted rights.

[clementval]

In D78340#1995153, @richard.barton.arm wrote:

@clementval - you should ask for commit access. I think already you have shown yourself to be a serious contributor, you should have no problem being granted rights.

@richard.barton.arm Will ask for it.

[kiranchandramohan]

I have just pushed this to the repo.
https://github.com/llvm/llvm-project/commit/a6c1692cb30e62c94513c2c6262cd76df92ef892

[clementval]

In D78340#1995265, @kiranchandramohan wrote:

I have just pushed this to the repo.
https://github.com/llvm/llvm-project/commit/a6c1692cb30e62c94513c2c6262cd76df92ef892

Thanks @kiranchandramohan. Meanwhile I have been granted commit access :-)

[ro]

This change broke -DBUILD_SHARED_LIBS=ON builds (seen on x86_64-pc-linux-gnu):

CMake Error: The inter-target dependency graph contains the following strongly connected component (cycle):
  "FortranEvaluate" of type SHARED_LIBRARY
    depends on "FortranSemantics" (weak)
  "FortranSemantics" of type SHARED_LIBRARY
    depends on "FortranEvaluate" (weak)
At least one of these targets is not a STATIC_LIBRARY.  Cyclic dependencies are allowed only among static libraries.

[clementval]

In D78340#1997821, @ro wrote:

This change broke -DBUILD_SHARED_LIBS=ON builds (seen on x86_64-pc-linux-gnu):

CMake Error: The inter-target dependency graph contains the following strongly connected component (cycle):
  "FortranEvaluate" of type SHARED_LIBRARY
    depends on "FortranSemantics" (weak)
  "FortranSemantics" of type SHARED_LIBRARY
    depends on "FortranEvaluate" (weak)
At least one of these targets is not a STATIC_LIBRARY.  Cyclic dependencies are allowed only among static libraries.

The cycle dependency was removed with D78215 which was breaking static build. The dependency is needed until it is really removed in the libraries themselves.

[DavidTruby]

As @clementval says there exists an actual cyclic dependency in the libraries that needs fixing, this patch didn't break it it only exposed an already existing issue.

[ro]

In D78340#1998243, @DavidTruby wrote:

As @clementval says there exists an actual cyclic dependency in the libraries that needs fixing, this patch didn't break it it only exposed an already existing issue.

Do you have a rough idea how long this might take? I'm asking because I try do debug a hanging testcase on Solaris/SPARC. A Release build
is useless for that if you need anything more than a stacktrace with just function names. Regular Debug builds have turned into a nightmare
with flang included, unfortunately: both assemble and link times have skyrocketed and diskspace consumption has gone through the roof
as well (72 GB on Solaris/SPARC, 81 GB on x86). A shared build (when it works) cuts diskspace for a Release almost in half and reduces it to
a quarter for Debug builds and massively helps for link times, too.

[DavidTruby]

In D78340#1999097, @ro wrote:

In D78340#1998243, @DavidTruby wrote:

As @clementval says there exists an actual cyclic dependency in the libraries that needs fixing, this patch didn't break it it only exposed an already existing issue.

Do you have a rough idea how long this might take? I'm asking because I try do debug a hanging testcase on Solaris/SPARC. A Release build
is useless for that if you need anything more than a stacktrace with just function names. Regular Debug builds have turned into a nightmare
with flang included, unfortunately: both assemble and link times have skyrocketed and diskspace consumption has gone through the roof
as well (72 GB on Solaris/SPARC, 81 GB on x86). A shared build (when it works) cuts diskspace for a Release almost in half and reduces it to
a quarter for Debug builds and massively helps for link times, too.

You might be able to get around some of this issue in debug builds by adding the following flag to your CXXFLAGS: "=gsplit-dwarf". It won't help with the size but it'll help a lot with the link times.

[ro]

Thanks for the hint. I'll give it a try: doesn't seem to have special requirements on the target other than using gas and gdb.

[ro]

Unfortunately, this didn't help as much as I hoped: when trying on Linux/x86_64, I got 113 GB for the -gsplit-dwarf build vs. 157 GB for a regular Debug build. Compare that with a shared build: only 23 GB.

Strange, bin/f18 is 1.8 GB with -gsplit-dwarf, but only 1.1 GB without.

FWIW, I discovered that there's a LLVM_USE_SPLIT_DWARF cmake option.

flang/lib/Evaluate/CMakeLists.txt

add_flang_library(FortranEvaluate
   static-data.cpp
   tools.cpp
   type.cpp
   variable.cpp
 
   LINK_LIBS
   FortranCommon
   FortranDecimal
+  FortranSemantics
   FortranParser
 )
 
 if (LIBPGMATH_DIR)
   # If pgmath library is found, it can be used for constant folding.
   find_library(LIBPGMATH pgmath PATHS ${LIBPGMATH_DIR})
   if(LIBPGMATH)
     add_compile_definitions(LINK_WITH_LIBPGMATH)
     target_link_libraries(FortranEvaluate ${LIBPGMATH})
     message(STATUS "Found libpgmath: ${LIBPGMATH}")
   else()
     message(STATUS "Libpgmath not found in: ${LIBPGMATH_DIR}")
   endif()
 endif()

flang/lib/Optimizer/Dialect/CMakeLists.txt

+get_property(dialect_libs GLOBAL PROPERTY MLIR_DIALECT_LIBS)
+
 add_flang_library(FIRDialect
   FIRAttr.cpp
   FIRDialect.cpp
   FIROps.cpp
   FIRType.cpp
 
   DEPENDS
   FIROpsIncGen
+  ${dialect_libs}
 
   LINK_LIBS
   FIRSupport
   MLIRTargetLLVMIR
   MLIRTargetLLVMIRModuleTranslation
   MLIREDSC
   MLIRExecutionEngine
   MLIRParser

flang/lib/Optimizer/Support/CMakeLists.txt

 get_property(dialect_libs GLOBAL PROPERTY MLIR_DIALECT_LIBS)
 
 add_flang_library(FIRSupport
   KindMapping.cpp
 
+  DEPENDS
+  ${dialect_libs}
   LINK_LIBS
   ${dialect_libs}
 )