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

Use shared libs for github actions #655

Open
gitoleg opened this issue Jun 4, 2024 · 8 comments
Open

Use shared libs for github actions #655

gitoleg opened this issue Jun 4, 2024 · 8 comments

Comments

@gitoleg
Copy link
Collaborator

gitoleg commented Jun 4, 2024

Maybe we should add one more github action and build with shared libraries as well.
It's not a very frequent case, but it happens sometimes - I mean when building with shared libraries may cause some problems.

This time (as far as I understand, though I may be wrong) this happens because of #638 and now we have a circular dependency:

CMake Error: The inter-target dependency graph contains the following strongly connected component (cycle):
  "clangCodeGen" of type SHARED_LIBRARY
    depends on "clangCIRFrontendAction" (weak)
  "clangCIRFrontendAction" of type SHARED_LIBRARY
    depends on "clangCodeGen" (weak)
    depends on "clangCIRLoweringDirectToLLVM" (weak)
    depends on "clangCIRLoweringThroughMLIR" (weak)
  "clangCIRLoweringDirectToLLVM" of type SHARED_LIBRARY
    depends on "clangCodeGen" (weak)
  "clangCIRLoweringThroughMLIR" of type SHARED_LIBRARY
    depends on "clangCodeGen" (weak)
At least one of these targets is not a STATIC_LIBRARY.  Cyclic dependencies are allowed only among static libraries.
CMake Generate step failed.  Build files cannot be regenerated correctly.

OR ... Am I missing something?

@bcardosolopes
Copy link
Member

I'm going to revert that PR, circular dependencies are not acceptable.

shared libraries as well

That'd be great. Do you know how to do it? I have no clue! (cc: @lanza)

bcardosolopes added a commit that referenced this issue Jun 6, 2024
This reverts commit f4d538f.

It's causing a circular dependency in shared lib builds. See #655
@bcardosolopes
Copy link
Member

@ChuanqiXu9 had to revert your PR, more context above ^

@ChuanqiXu9
Copy link
Member

Oh, got it. No problem at all. I'll try to look into that when I have time.

@gitoleg
Copy link
Collaborator Author

gitoleg commented Jun 7, 2024

That'd be great. Do you know how to do it? I have no clue!

@bcardosolopes well, you need to add -DBUILD_SHARED_LIBS=ON in your cmake args.

@bcardosolopes
Copy link
Member

That'd be great. Do you know how to do it? I have no clue!

@bcardosolopes well, you need to add -DBUILD_SHARED_LIBS=ON in your cmake args.

My question was regarding how to enable that for github testing to pick up on it. If you know what needs to change I'm happy approving a PR

@gitoleg
Copy link
Collaborator Author

gitoleg commented Jun 11, 2024

@bcardosolopes
got it)
well, it's been a while since I touched github actions (8 years, omg! :) ) but I can do it!. But we need to fix the build itself first.

Now the problem is that clangCIR depends on MLIRCIRTransforms , and MLIRCIRTransforms depends on TargetLowering that used CIRDataLayout which is in clangCIR .

I'm not sure that's good that CIR codegen somehow depends on MLIRCIRTransforms. Maybe we can place CIRPasses.cpp somewhere else, I don't know.

So it's really a problem. And I don't know, if I'm the only who uses build with shared lib (though it's faster! :) ) But the longer we don't have such test build the more problems we will have in future.

@bcardosolopes
Copy link
Member

well, it's been a while since I touched github actions (8 years, omg! :) ) but I can do it!.

Awesome!

Now the problem is that clangCIR depends on MLIRCIRTransforms , and MLIRCIRTransforms depends on TargetLowering that used CIRDataLayout which is in clangCIR .

@sitio-couto looks like target lowering introduced a cycle. Please make sure you prioritize solving this problem before you land more work, or at least soon enough. Cycles are not acceptable. Might be worth checking shared builds in your workflow as you add more of these abstractions.

So it's really a problem. And I don't know, if I'm the only who uses build with shared lib (though it's faster! :) ) But the longer we don't have such test build the more problems we will have in future.

This also affects how we use our compiler internally, so you are not alone.

@sitio-couto
Copy link
Collaborator

@bcardosolopes @gitoleg #687

@gitoleg could you double-check that this PR builds with BUILD_SHARED_LIBS=ON please?

pysuxing pushed a commit to pysuxing/llvm-project that referenced this issue Jul 17, 2024
This reverts commit f4d538f.

It's causing a circular dependency in shared lib builds. See llvm/clangir#655
bcardosolopes pushed a commit that referenced this issue Sep 18, 2024
Reland #638

This was reverted due to #655. I
tried to address the problem in the newest commit.

The changes of the PR since the last landed one includes:
- Move the definition of `cir::CIRGenConsumer` to
`clang/include/clang/CIRFrontendAction/CIRGenConsumer.h`, and leave its
`HandleTranslationUnit` interface is left empty. So that
`cir::CIRGenConsumer` won't need to depend on CodeGen any more.
- Change the old definition of `cir::CIRGenConsumer` in
`clang/lib/CIR/FrontendAction/CIRGenAction.cpp` and to
`CIRLoweringConsumer`, inherited from `cir::CIRGenConsumer`, which
implements the original `HandleTranslationUnit` interface.

I feel this may improve the readability more even without my original
patch.
Hugobros3 pushed a commit to shady-gang/clangir that referenced this issue Oct 2, 2024
This reverts commit f4d538f.

It's causing a circular dependency in shared lib builds. See llvm#655
Hugobros3 pushed a commit to shady-gang/clangir that referenced this issue Oct 2, 2024
Reland llvm#638

This was reverted due to llvm#655. I
tried to address the problem in the newest commit.

The changes of the PR since the last landed one includes:
- Move the definition of `cir::CIRGenConsumer` to
`clang/include/clang/CIRFrontendAction/CIRGenConsumer.h`, and leave its
`HandleTranslationUnit` interface is left empty. So that
`cir::CIRGenConsumer` won't need to depend on CodeGen any more.
- Change the old definition of `cir::CIRGenConsumer` in
`clang/lib/CIR/FrontendAction/CIRGenAction.cpp` and to
`CIRLoweringConsumer`, inherited from `cir::CIRGenConsumer`, which
implements the original `HandleTranslationUnit` interface.

I feel this may improve the readability more even without my original
patch.
smeenai pushed a commit to smeenai/clangir that referenced this issue Oct 9, 2024
This reverts commit f4d538f.

It's causing a circular dependency in shared lib builds. See llvm#655
smeenai pushed a commit to smeenai/clangir that referenced this issue Oct 9, 2024
Reland llvm#638

This was reverted due to llvm#655. I
tried to address the problem in the newest commit.

The changes of the PR since the last landed one includes:
- Move the definition of `cir::CIRGenConsumer` to
`clang/include/clang/CIRFrontendAction/CIRGenConsumer.h`, and leave its
`HandleTranslationUnit` interface is left empty. So that
`cir::CIRGenConsumer` won't need to depend on CodeGen any more.
- Change the old definition of `cir::CIRGenConsumer` in
`clang/lib/CIR/FrontendAction/CIRGenAction.cpp` and to
`CIRLoweringConsumer`, inherited from `cir::CIRGenConsumer`, which
implements the original `HandleTranslationUnit` interface.

I feel this may improve the readability more even without my original
patch.
smeenai pushed a commit to smeenai/clangir that referenced this issue Oct 9, 2024
Reland llvm#638

This was reverted due to llvm#655. I
tried to address the problem in the newest commit.

The changes of the PR since the last landed one includes:
- Move the definition of `cir::CIRGenConsumer` to
`clang/include/clang/CIRFrontendAction/CIRGenConsumer.h`, and leave its
`HandleTranslationUnit` interface is left empty. So that
`cir::CIRGenConsumer` won't need to depend on CodeGen any more.
- Change the old definition of `cir::CIRGenConsumer` in
`clang/lib/CIR/FrontendAction/CIRGenAction.cpp` and to
`CIRLoweringConsumer`, inherited from `cir::CIRGenConsumer`, which
implements the original `HandleTranslationUnit` interface.

I feel this may improve the readability more even without my original
patch.
smeenai pushed a commit to smeenai/clangir that referenced this issue Oct 9, 2024
Reland llvm#638

This was reverted due to llvm#655. I
tried to address the problem in the newest commit.

The changes of the PR since the last landed one includes:
- Move the definition of `cir::CIRGenConsumer` to
`clang/include/clang/CIRFrontendAction/CIRGenConsumer.h`, and leave its
`HandleTranslationUnit` interface is left empty. So that
`cir::CIRGenConsumer` won't need to depend on CodeGen any more.
- Change the old definition of `cir::CIRGenConsumer` in
`clang/lib/CIR/FrontendAction/CIRGenAction.cpp` and to
`CIRLoweringConsumer`, inherited from `cir::CIRGenConsumer`, which
implements the original `HandleTranslationUnit` interface.

I feel this may improve the readability more even without my original
patch.
keryell pushed a commit to keryell/clangir that referenced this issue Oct 19, 2024
This reverts commit f4d538f.

It's causing a circular dependency in shared lib builds. See llvm#655
keryell pushed a commit to keryell/clangir that referenced this issue Oct 19, 2024
Reland llvm#638

This was reverted due to llvm#655. I
tried to address the problem in the newest commit.

The changes of the PR since the last landed one includes:
- Move the definition of `cir::CIRGenConsumer` to
`clang/include/clang/CIRFrontendAction/CIRGenConsumer.h`, and leave its
`HandleTranslationUnit` interface is left empty. So that
`cir::CIRGenConsumer` won't need to depend on CodeGen any more.
- Change the old definition of `cir::CIRGenConsumer` in
`clang/lib/CIR/FrontendAction/CIRGenAction.cpp` and to
`CIRLoweringConsumer`, inherited from `cir::CIRGenConsumer`, which
implements the original `HandleTranslationUnit` interface.

I feel this may improve the readability more even without my original
patch.
lanza pushed a commit that referenced this issue Nov 5, 2024
This reverts commit f4d538f.

It's causing a circular dependency in shared lib builds. See #655
lanza pushed a commit that referenced this issue Nov 5, 2024
Reland #638

This was reverted due to #655. I
tried to address the problem in the newest commit.

The changes of the PR since the last landed one includes:
- Move the definition of `cir::CIRGenConsumer` to
`clang/include/clang/CIRFrontendAction/CIRGenConsumer.h`, and leave its
`HandleTranslationUnit` interface is left empty. So that
`cir::CIRGenConsumer` won't need to depend on CodeGen any more.
- Change the old definition of `cir::CIRGenConsumer` in
`clang/lib/CIR/FrontendAction/CIRGenAction.cpp` and to
`CIRLoweringConsumer`, inherited from `cir::CIRGenConsumer`, which
implements the original `HandleTranslationUnit` interface.

I feel this may improve the readability more even without my original
patch.
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

No branches or pull requests

4 participants