Skip to content

Prevent linker optimization affecting dialect registration - #9

Open
bmahjour wants to merge 3 commits into
mainfrom
bmahjour/fix-dialect-registration
Open

bmahjour wants to merge 3 commits into
mainfrom
bmahjour/fix-dialect-registration

Conversation

@bmahjour

Copy link
Copy Markdown
Collaborator

MLIR relies heavily on static initialization of states in global constructors. Some linkers may throw away symbols that are only referenced in static initializers since they are not part of any call chain starting from main. This causes the following error (on MacOS):

LLVM ERROR: can't create Attribute 'mlir::DenseArrayAttr' because storage uniquer isn't initialized: the dialect was likely not loaded, or the attribute wasn't added with addAttributes<...>() in the Dialect::initialize() method.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace and instructions to reproduce the bug.
Stack dump:

This PR

  1. adds linker options -all_load or --whole-archive to force linkers to keep all symbols in MLIR libraries.
  2. Uses selective dialect registration via registry.insert<>() rather than mlir::registerAllDialects() to only register dialects that are actually needed.

@bmahjour bmahjour self-assigned this Apr 21, 2026
@bmahjour bmahjour mentioned this pull request Apr 21, 2026
… in MLIR's static initializers + only register dependent dialects (not all)

Signed-off-by: Bardia Mahjour <bmahjour@ca.ibm.com>
@bmahjour
bmahjour force-pushed the bmahjour/fix-dialect-registration branch from 2f24ec4 to e3afd26 Compare April 21, 2026 15:59
Signed-off-by: Bardia Mahjour <bmahjour@ca.ibm.com>
@fabianlim

fabianlim commented Apr 21, 2026

Copy link
Copy Markdown
Collaborator

@bmahjour thank you for the PR, if you dont mind can I clarify your following statement

MLIR relies heavily on static initialization of states in global constructors. Some linkers may throw away symbols that are only referenced in static initializers since they are not part of any call chain starting from main. This causes the following error (on MacOS):

may I check which MLIR version are you facing this error? I have built this repo on my Mac as well and I do not see this error for newer LLVM versions. If you would like, the following Visual Studio devcontainer build, uses a quite recent 20260409+3b3ac5a11 version of mlir-wheel.

This devcontainer build can be found in my fork, and its quite easy to try

  • checkout the above branch
  • in VSCode, do Dev Container: Rebuild Container and it will open the folder in a container with the mlir and the ktir-mlir-frontend installed.
  • if you do this on Mac, it will pull an aarch build of mlir-wheel, which is built for the arm architecture (assuming you are on M* silicon)
  • you can run all the tests as described in the README and they will pass.
image
  • The reason why im bringing up the newer LLVM versions, is that there is a possiblity we may be moving towards that, see this issue
  • However, I will also take a closer look at your proposed changes, but I thought I just brought up the above point, because I just wanted to clarify if this might be something you wanted to change to make it compatible with an older LLVM version.
    • My guess is that you are seeing this error on LLVM 22.1.0 4434dabb69916856b824f68a64b029c67175e532 on Mac (I havnt tried this combi yet)
    • LLVM 22.1.0 on x86 should be passing, as seen here

@bmahjour

Copy link
Copy Markdown
Collaborator Author

We are hitting this error with LLVM 22.1.0 4434dabb69916856b824f68a64b029c67175e532 on Mac with the following build compiler:
Apple clang version 17.0.0 (clang-1700.6.4.2)
Target: arm64-apple-darwin25.3.0

@fabianlim

fabianlim commented Apr 22, 2026

Copy link
Copy Markdown
Collaborator

@bmahjour ok i created an issue #11 to add a stable workflow to the CI to build against macOS and clang so that we an catch issues on macOs

@mudhakar there could be a possibility (see issue here) that we would settle on a stable LLVM version, and given that there are users like @bmahjour who are building against these versions on mac, perhaps it would be good to add a CI workflow to test against.

If we decide that we will go to bleeding-edge, we can deprecate this ci workflow; it is something that could be useful in the interim

@fabianlim

fabianlim commented Apr 23, 2026

Copy link
Copy Markdown
Collaborator

@bmahjour just digging abit deeper into this issue, I tried building on the same LLVM 22.1.0 4434dabb6, taking the official tarball , and build it on a macos runner, it does build, see the lit tests running here.

I think your problem is your apple clang compiler is too old, what I did was to rely on the clang compiler distributed in the tarball, see this. I did originally see linker problems, but when I did the above it was resolved.

Run LIT=$(uv run --no-project which lit)
  LIT=$(uv run --no-project which lit)
  cmake -S . -B build -GNinja \
    -DCMAKE_BUILD_TYPE=Release \
    -DKTIR_ENABLE_PYTHON_BINDINGS=OFF \
    -DMLIR_DIR=/Users/runner/work/ktir-mlir-frontend/ktir-mlir-frontend/llvm-release/lib/cmake/mlir \
    -DLLVM_EXTERNAL_LIT=$LIT \
    -DLLVM_LIT_ARGS="-v"
  shell: /bin/bash -e {0}
  env:
    LLVM_VERSION: 22.1.0
    MLIR_DIR: /Users/runner/work/ktir-mlir-frontend/ktir-mlir-frontend/llvm-release/lib/cmake/mlir
    CC: /Users/runner/work/ktir-mlir-frontend/ktir-mlir-frontend/llvm-release/bin/clang
    CXX: /Users/runner/work/ktir-mlir-frontend/ktir-mlir-frontend/llvm-release/bin/clang++
    UV_CACHE_DIR: /Users/runner/work/_temp/setup-uv-cache
-- The CXX compiler identification is Clang 22.1.0
-- The C compiler identification is Clang 22.1.0

@bmahjour

Copy link
Copy Markdown
Collaborator Author

The link-time optimization behavior may be different across different linkers (eg. lld vs gnu ld) and may also change from release to release for a given linker. The proposed changes in this PR ensure we get consistent behavior.

@fabianlim

fabianlim commented Apr 24, 2026

Copy link
Copy Markdown
Collaborator

The link-time optimization behavior may be different across different linkers (eg. lld vs gnu ld) and may also change from release to release for a given linker. The proposed changes in this PR ensure we get consistent behavior.

@bmahjour thank you. Do you mind if I update the branch so that we can test against the latest CI? Maybe would be good to have #14 merged first and we can test these changes on 3 setups.

I verified these changes locally in my mac on v22.1.3.

@fabianlim

Copy link
Copy Markdown
Collaborator

@bmahjour thank you for the PR and for investigating this issue. After discussion, we'd like to share our findings and concerns with @mudhakar @raghukiran1224 @kiszk @Prasanth-Chatarasi before proceeding.

The bug is not reproducible in our CI

We ran the unmodified main branch (without this PR's changes) on macOS 15 ARM64 using the same Apple Clang version you reported the issue with — AppleClang 17.0.0.17000013:

Both runs use the GitHub-hosted macos-15-arm64 runner with the system compiler. Since the baseline already passes on the same Apple Clang version, the DenseArrayAttr error appears to be specific to your local environment (possibly a different Xcode SDK version, custom CMake flags, or a differently-configured LLVM build) rather than a systematic Apple Clang issue.

Concerns with the proposed changes

Even setting aside reproducibility, we have several concerns with the changes as proposed:

  1. -all_load is too broad on macOS. Unlike Linux where --whole-archive/--no-whole-archive can bracket specific libraries, macOS -all_load is a global flag that forces linking of every object in every archive — including KTDP_LIBS and anything else on the link line. The macOS equivalent for selective forcing is -force_load <path> per archive.

  2. PRIVATE is specified twice. set(DEPS PRIVATE -Wl,--whole-archive ...) embeds PRIVATE in the variable, and then target_link_libraries(ktir-opt PRIVATE ${DEPS}) adds it again. This is a bug in the CMake.

  3. registerAllPasses() was removed with no replacement. The PR removes mlir::registerAllPasses() but doesn't register any passes selectively. If any lit test or user invocation relies on standard MLIR passes (e.g., --canonicalize, --cse), it would fail at runtime. The current tests happen to pass because they only exercise dialect parsing/printing, but this would break as soon as pass-based tests are added.

  4. No fallback for other platforms. The if/elseif in CMake only handles Linux and Darwin. On any other platform, DEPS is empty and target_link_libraries links nothing — a silent build failure.

Recommendation

We'd prefer not to merge this as-is, but we're open to revisiting if we can validate the changes against downstream consumers. ktir-cpu PR #24 is in the process of switching its ktir-mlir-frontend dependency from a fork to upstream — once that merges, ktir-cpu will build directly against this repo's main, and any changes here would flow downstream. We'd want to verify that the linker flag and dialect registration changes don't break ktir-cpu's full test suite (520+ tests) before merging.

We'd also like other maintainers to weigh in with their thoughts on this.

@KFAFSP

KFAFSP commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Since there is now a new build system, this PR is outdated, but its contents are still relevant. Note that we're usually building against an MLIR DSO, which does not suffer from this issue.

There's actually a second angle to this, which is downstream ODR violations. As long as this PR is only about changing the linking behavior of the tool executable, there is no problem there. However, in the case of linking on other shared artifacts, the usage of whole-archive when combined with MLIR_LINK_MLIR_DYLIB causes great headaches. These include ODR violations of dialect TypeIDs leading to errors. I've already had to work around these for the PythonCAPIs, which are implemented in MLIR's CMake with a similar mechanism. See here for the work-around.

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

Successfully merging this pull request may close these issues.

3 participants