Support standalone builds (work-in-progress) #1245

mgorny · 2024-12-19T12:29:05Z

Per llvm/llvm-project#119130. This is incomplete and non-working at this point, but I'd like some early feedback on the approach, especially since it is a bit tedious. I have included llvm/llvm-project#119408 too, since it made linking much easier via mlir_target_link_libraries.

This is the following up fix for the previous fix llvm#961 See the attached new test for the reproducer. Sorry for the initial overlook.

- The flag is the default even for cc1, so make it disable two level deep. - While here, remove the unnecessary flag disable for pure `-emit-cir`.

…nv-lowering While here, add more unrecheables to cover some of the current errors, so that our users can see a clear message instead of a random cast assert of sorts. This covers at least all crashes seen when removing -fno-clangir-call-conv-lowering from all tests, there are probably other things we'll find as we exercise this path.

These are not meant to be used by any other component, make sure it's very specific.

This is the usual copy-paste-modify from CodeGen, though I changed all the variable names to conform to our new style. All these functions should be pulled out as common helpers when we're upstream.

llvm/llvm-project@1d0bd8e moves a conditional from CodeGen to AST, and this follows suit for consistency. (Our support for the Microsoft ABI is NYI anyway; this is just to make things simpler to follow when matching up logic between CodeGen and CIRGen.)

There is no change to testing functionality. This refacot let those files have the same Run options that is easier to maintain and extend.

…#976) Close llvm#975 See the attached test case for example

This PR adds initial support for the `__int128` type. The `!cir.int` type is extended to support 128-bit integer types. This PR comes with a simple test that verifies the CIRGen and LLVM lowering of `!s128i` and `!u128i` work. Resolve llvm#953 .

…ll (llvm#982)

…ulhq_lane and vqrdmulh_lane (llvm#985)

…d (in CIR + Direct to LLVM) (llvm#966) Fixes llvm#931 Added type definition in CIRTypes.td, created appropriate functions for the same in CIRTypes.cpp like getPreferredAlignment, getPreferredAlignment, etc. Optionally added lowering in LowerToLLVM.cpp

… pipeline This is causing lots of churn. `-fclangir-call-conv-lowering` is not mature enough, assumptions are leading to crashes we cannot track with special messages, leading to not great user experience. Turn this off until we have someone dedicated to roll this out.

While here add some bits for ptr auth and match OG.

…lvm#990) LLVM's verifier enforces this, which was previously causing us to fail verification. This is a bit of a band-aid; the overall linkage and visibility setting flow needs some work to match the original.

…lvm#965) As title, but important step in this PR is to allow CIR ShiftOp to take vector of int type as input type. As result, I added a verifier to ShiftOp with 2 constraints 1. Input type either all vector or int type. This is consistent with LLVM::ShlOp, vector shift amount is expected. 2. In the spirit of C99 6.5.7.3, shift amount type must be the same as result type, the if vector type is used. (This is enforced in LLVM lowering for scalar int type).

…iltinExpr (llvm#967) This PR helps us to triage unimplemented builtins (that are target independent). There are unhandled builtins in CIR Codegen `[CIRGenFunction::buildBuiltinExpr](https://github.com/llvm/clangir/blob/4c446b3287895879da598e23164d338d04bced3e/clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp#L305)`. And those builtins have implementation in [OG](https://github.com/llvm/clangir/blob/4c446b3287895879da598e23164d338d04bced3e/clang/lib/CodeGen/CGBuiltin.cpp#L2573). Currently, those builtins just are treated as LibraryCall or some other ways which eventually get failure, and failure messages are confusing. This PR address this problem by refactoring `CIRGenFunction::buildBuiltinExpr` to keep the same skeleton as OG counterpart `CodeGenFunction::EmitBuiltinExpr`, and add builtin name to NYI message

…n_vqsub (llvm#988)

Add more NFC skeleton while here.

…#1239) This implementation is different from OG in the sense we chose to use CIR op which eventually lowers to generic LLVM intrinsics instead of llvm.aarch64.neon intrinsics But down to the ASM level, [they are identical ](https://godbolt.org/z/Gbbos9z6Y).

…vm#1242) This patch follows llvm#1220 (comment) by augmenting `CIR_Type` with a new field, `tbaaName`. Specifically, it enables TableGen support for the `-gen-cir-tbaa-name-lowering` option, allowing for the generation of `getTBAAName` functions based on the `tbaaName`. This enhancement enables us to replace the hardcoded TBAA names in the `getTypeName` function with the newly generated `getTBAAName`.

keryell · 2024-12-19T17:24:31Z

I guess we should rebase ClangIR on top of latest LLVM to have llvm/llvm-project#119408 in it and simplify this PR first?

This PR adds a bitcast when we rewrite globals type. Previously we just set a new type and it worked. But recently I started to test ClangIR with CSmith in order to find some run time bugs and faced with the next problem. ``` typedef struct { int x : 15; uint8_t y; } S; S g = { -12, 254}; int main() { printf("%d\n", g.y); return 0; } ``` The output for this program is ... 127 but not 254! The reason is that first global var is created with the type of struct `S`, then `get_member` operation is generated with index `1` and then after, the type of the global is rewritten - I assume because of the anon struct created on the right side in the initialization. But the `get_member` operation still wants to access to the field at index `1` and get a wrong byte. If we change the `y` type to `int` we will fail on the verification stage. But in the example above it's a run time error! This is why I suggest to add a bitcast once we have to rewrite the global type.

…2/u32, (llvm#1240) Co-authored-by: Bruno Cardoso Lopes <[email protected]>

We figure it would be nice to have a common place with all our known crashes that is tracked by git and is actively verified whether or not we can now support the crashes by lit. It can act as our source of truth for known failures and also being potential good first tasks for new developers. Add a simple test case of a known crash that involves copying a struct in a catch. Reviewers: smeenai, bcardosolopes Reviewed By: bcardosolopes Pull Request: llvm#1243

…#1247) Basically that is - the return value for `=` operator for bitfield assignment is wrong now. For example, the next function returns `7` for 3 bit bit field, though it should be `-1`: ``` int get_a(T *t) { return (t->a = 7); } ``` This PR fix it. Actually, the bug was in the lowering - the integer cast is applied in the wrong place (in comparison with the original codegen).

…Name (llvm#1242)" Need to revert llvm#1220 This reverts commit 445f2a5.

Fix llvm#1246 This reverts commit c94c04e.

mgorny · 2024-12-22T12:02:07Z

I've gotten clang itself working — or at least it seems to compile trivial programs with -fclangir, and the examples seem to print something that looks ClangIR-y. I'm working on tests now. Once I'm done, I'll rebase on top of ClangIR main.

While MLIR currently supports building a libMLIR.so, it does not support actually linking against it for its own tools. When building with LTO, this means we have to relink the world for every tool, and the resulting binaries are large. This adds basic support for MLIR_LINK_MLIR_DYLIB, modelled after how CLANG_LINK_CLANG_DYLIB is implemented: Libraries that are part of libMLIR.so should be added via mlir_target_link_libraries instead of target_link_libraries. This will replace them with libMLIR.so if MLIR_LINK_MLIR_DYLIB is enabled. This adds basic support, I think there are two more things that can be done here: * C API unit tests should link against libMLIR-C.so. Currently these still link statically. * Linking the test libs (not part of libMLIR.so) still pulls in dependencies statically that should come from libMLIR.so.

This commit combines three changes needed for ClangIR to be built successfully on Gentoo with standalone Clang/MLIR install: 1. It unblocks `CLANG_ENABLE_CIR` + `CLANG_BUILT_STANDALONE` scenario, obtaining the necessary bits from system-wide installation of MLIR via `find_package(MLIR)`. 2. It switches linking to MLIR libraries to use `mlir_target_link_libraries()` introduced in 10ef20f, which supports both linking to invidual MLIR static libraries and to the shared libMLIR library. 3. It makes dependencies on MLIR-specific targets conditional to non-standalone builds -- since these targets aren't present in clang build tree when it is built standalone. These changes do not cover tests yet.

The only change needed was making the dependency on mlir-translate target optional, as mlir-translate is installed as part of mlir when building standalone.

mgorny · 2024-12-22T13:14:07Z

Ok, it turned out easier than I anticipated. I think it's fine to merge it as-is, including the mlir backport, since the next rebase will simply "eat" it, or rebase-then-merge, if you prefer.

lanza · 2024-12-23T06:11:32Z

With the invocation:

cmake -Sclang -Bclang/build/Release -DCMAKE_BUILD_TYPE=Release -DLLVM_DIR=build/Release/lib/cmake -DCLANG_ENABLE_CIR=YES -GNinja

on Linux-x86_64 I end up with

CMake Error at lib/FrontendTool/CMakeLists.txt:56 (mlir_target_link_libraries):
  Unknown CMake command "mlir_target_link_libraries".

The LLVM build does include MLIR. Is this the right standalone invocation?

mgorny · 2024-12-23T06:14:25Z

It's a 3-step "total standalone":

Build and install LLVM.
Build and install MLIR (using installed LLVM).
Build and install Clang (using installed LLVM and MLIR).

mgorny · 2024-12-23T06:15:37Z

I mean, I suppose it will also work if you include MLIR in your LLVM build, i.e. -DLLVM_ENABLE_PROJECTS='llvm;mlir' but I haven't tried that, as it kinda defeats the purpose of standalone building.

bcardosolopes · 2025-01-06T15:03:26Z

I'll defer this review to both @lanza and @smeenai

smeenai

Sorry for the late review, I missed this earlier.

We'll want to cherry-pick llvm/llvm-project#119408 separately, to make this easier to review.

smeenai · 2025-01-13T17:58:38Z

clang/lib/CIR/CodeGen/CMakeLists.txt

@@ -52,16 +57,19 @@ add_clang_library(clangCIR
  MLIRCIRASTAttrInterfacesIncGen
  MLIRCIROpInterfacesIncGen
  MLIRCIRLoopOpInterfaceIncGen
-  ${dialect_libs}
+  ${deps}


Why do we need this? Is the mlir_target_link_libraries below not enough? The same question applies everywhere else this pattern is followed.

I don't know why the explicit dependencies were added in the first place. I wanted to avoid breaking something.

Got it. I suspect it was mostly copy-pasting from elsewhere, and this is a good opportunity to clean that up :)

smeenai · 2025-01-13T18:29:26Z

clang/lib/CIR/Lowering/DirectToLLVM/CMakeLists.txt

  MLIRCIR
+  )
+mlir_target_link_libraries(clangCIRLoweringDirectToLLVM PUBLIC


Nit: missing newline

smeenai · 2025-01-13T19:10:47Z

clang/tools/cir-lsp-server/CMakeLists.txt

 add_mlir_tool(cir-lsp-server
  cir-lsp-server.cpp

  DEPENDS
-  ${LIBS}
+  ${DEPS}


In general, I think we want to only be linking the MLIR libs with mlir_target_link_libraries and not under DEPENDS, so that we link against libMLIR.dylib correctly.

ChuanqiXu9 and others added 30 commits November 22, 2024 18:25

[CIR] [Lowering] Fix handling of multiple array for ZeroAttr (llvm#970)

5f9b3b8

This is the following up fix for the previous fix llvm#961 See the attached new test for the reproducer. Sorry for the initial overlook.

[CIR][Driver] Fix -fclangir-call-conv-lowering behavior

cb888fa

- The flag is the default even for cc1, so make it disable two level deep. - While here, remove the unnecessary flag disable for pure `-emit-cir`.

[CIR] Add more user facing messages for -fno-clangir-call-conv-lowering

7c851c3

[CIR][NFC] Massively rename workarounds for callconv lowering

dd9423f

These are not meant to be used by any other component, make sure it's very specific.

[CIR][CIRGen] Implement CIRGenModule::shouldEmitFunction (llvm#984)

761c564

This is the usual copy-paste-modify from CodeGen, though I changed all the variable names to conform to our new style. All these functions should be pulled out as common helpers when we're upstream.

[CIR][LowerToLLVM] Lower cir.vtt.address_point

8a12125

[CIR][NFC] Rename function in a test

26a1cae

[CIR][CIRGen] Get more vtable and dtor working

d8fc11b

[CIR][CIRGen][NFC] Consolidate RUN lines for builtin tests (llvm#968)

1091422

There is no change to testing functionality. This refacot let those files have the same Run options that is easier to maintain and extend.

[CIR] [Lowering] care trailing zero for lowering constant array (llvm…

11cbb68

…#976) Close llvm#975 See the attached test case for example

[CIR][CIRGen][Builtin][Neon] Lower neon_vrshrn_n to llvm intrinsic ca…

bafe969

…ll (llvm#982)

[CIR][CIRGen][Builtin][Neon] Lower vqdmulhq_lane, vqdmulh_lane, vqrdm…

526f3b7

…ulhq_lane and vqrdmulh_lane (llvm#985)

[CIR][NFC] Fix some consistency issues with missing features

4183dde

[CIR][CIRGen][Builtin] Implement builtin addressof (llvm#987)

0522f59

[CIR][NFC] Improve documentation about goto's

f6f02d1

[CIR][NFC] Move code around to match OG

cb384db

[CIR][CIRGen][NFC] Add more skeleton for handling inheritance ctors

8c92d88

While here add some bits for ptr auth and match OG.

[CIR][CIRGen] Ensure default visibility for local linkage functions (l…

da80c19

…lvm#990) LLVM's verifier enforces this, which was previously causing us to fail verification. This is a bit of a band-aid; the overall linkage and visibility setting flow needs some work to match the original.

[CIR][CIRGen][Builtin][Neon] Lower neon_vqaddq_v, neon_vqsubq and neo…

85e47c1

…n_vqsub (llvm#988)

[CIR][CIRGen][Builtin][Neon] Lower neon_vmovl_v (llvm#989)

9415d8a

[CIR][CIRGen] Support initial cases of inheritance ctor/dtor

dbddba6

Add more NFC skeleton while here.

[CIR][CIRGen] Null init some inheritance components

c6ae220

[CIR][CIRGen][NFC] More skeleton for building constants

da31d2c

ghehg and others added 3 commits December 18, 2024 08:51

[CIR][NFC] Silence warning from recent PR

2ec1a24

gitoleg and others added 7 commits December 19, 2024 09:37

[CIR][CIRGen][Builtin][Neon] Lower neon_vaddvq_s64/s32, neon_vaddv_s3…

363440c

…2/u32, (llvm#1240) Co-authored-by: Bruno Cardoso Lopes <[email protected]>

Revert "[CIR][CIRGen][TBAA] Replace hardcoded TBAA names with getTBAA…

adae4ff

…Name (llvm#1242)" Need to revert llvm#1220 This reverts commit 445f2a5.

Revert "[CIR][CIRGen][TBAA] Add support for scalar types (llvm#1220)"

c921ec0

Fix llvm#1246 This reverts commit c94c04e.

[CIR][Types] Mimic OG codegen on isFuncParamTypeConvertible

04d7dcf

mgorny force-pushed the cir-standalone branch 3 times, most recently from fa15e8f to 154a71a Compare December 22, 2024 11:15

nikic and others added 3 commits December 22, 2024 13:45

[CIR] Support running tests in standalone builds

9b9353a

The only change needed was making the dependency on mlir-translate target optional, as mlir-translate is installed as part of mlir when building standalone.

mgorny force-pushed the cir-standalone branch from c70cb63 to 9b9353a Compare December 22, 2024 13:12

mgorny marked this pull request as ready for review December 22, 2024 13:12

mgorny requested review from lanza and bcardosolopes as code owners December 22, 2024 13:12

bcardosolopes requested a review from smeenai January 6, 2025 15:03

smeenai reviewed Jan 13, 2025

View reviewed changes

lanza force-pushed the main branch from 9ffbe92 to 3c3b096 Compare January 27, 2025 22:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support standalone builds (work-in-progress) #1245

Support standalone builds (work-in-progress) #1245

mgorny commented Dec 19, 2024

keryell commented Dec 19, 2024

mgorny commented Dec 22, 2024

mgorny commented Dec 22, 2024

lanza commented Dec 23, 2024

mgorny commented Dec 23, 2024

mgorny commented Dec 23, 2024

bcardosolopes commented Jan 6, 2025

smeenai left a comment

smeenai Jan 13, 2025

mgorny Jan 13, 2025

smeenai Jan 13, 2025

smeenai Jan 13, 2025

smeenai Jan 13, 2025

Support standalone builds (work-in-progress) #1245

Are you sure you want to change the base?

Support standalone builds (work-in-progress) #1245

Conversation

mgorny commented Dec 19, 2024

keryell commented Dec 19, 2024

mgorny commented Dec 22, 2024

mgorny commented Dec 22, 2024

lanza commented Dec 23, 2024

mgorny commented Dec 23, 2024

mgorny commented Dec 23, 2024

bcardosolopes commented Jan 6, 2025

smeenai left a comment

Choose a reason for hiding this comment

smeenai Jan 13, 2025

Choose a reason for hiding this comment

mgorny Jan 13, 2025

Choose a reason for hiding this comment

smeenai Jan 13, 2025

Choose a reason for hiding this comment

smeenai Jan 13, 2025

Choose a reason for hiding this comment

smeenai Jan 13, 2025

Choose a reason for hiding this comment