Page MenuHomePhabricator

[Flang] fix dependency issues after D78215
ClosedPublic

Authored by clementval on Apr 16 2020, 6:09 PM.

Details

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....

Diff Detail

Event Timeline

clementval created this revision.Apr 16 2020, 6:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2020, 6:09 PM
clementval edited the summary of this revision. (Show Details)Apr 16 2020, 6:10 PM
DavidTruby accepted this revision.Apr 17 2020, 3:30 AM

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.

This revision is now accepted and ready to land.Apr 17 2020, 3:30 AM
ChinouneMehdi added inline comments.
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!

ChinouneMehdi added a project: Restricted Project.Apr 17 2020, 4:48 AM
clementval marked 3 inline comments as done.Apr 17 2020, 4:53 AM
clementval added inline comments.
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.

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?

This comment was removed by ChinouneMehdi.
ChinouneMehdi added inline comments.Apr 17 2020, 6:09 AM
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 marked 2 inline comments as done.Apr 17 2020, 6:34 AM

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).

clementval retitled this revision from [Flang] fix dependenvy issues after D78215 to [Flang] fix dependency issues after D78215.Apr 17 2020, 6:38 AM
ChinouneMehdi added a subscriber: ChinouneMehdi.

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 marked an inline comment as done.Apr 17 2020, 7:03 AM
clementval added a subscriber: ChinouneMehdi.

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 added inline comments.
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 marked an inline comment as done.Apr 17 2020, 7:39 AM
clementval added inline comments.
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.

@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 added inline comments.Apr 17 2020, 7:55 AM
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 marked an inline comment as done.Apr 17 2020, 7:58 AM
clementval added inline comments.
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 added inline comments.Apr 17 2020, 7:59 AM
flang/lib/Optimizer/Support/CMakeLists.txt
6–7

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

clementval edited the summary of this revision. (Show Details)

Remove indirect dependency and use direct one.

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 marked an inline comment as done.Apr 17 2020, 8:39 AM
clementval added inline comments.
flang/lib/Optimizer/Dialect/CMakeLists.txt
11

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

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.

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 :)

@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 accepted this revision.Apr 21 2020, 8:38 AM

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?

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.

@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 - 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.

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

This revision was automatically updated to reflect the committed changes.
ro added a subscriber: ro.Apr 22 2020, 2:11 PM

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.
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.

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 added a comment.Apr 23 2020, 6:59 AM

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.

In D78340#1999097, @ro 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 added a comment.Apr 23 2020, 7:36 AM

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 added a comment.Apr 23 2020, 12:22 PM

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.