Skip to content

llvm: Use inline variants of memcpy/memset intrinsics when using -fno-builtin #22903

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

Merged
merged 2 commits into from
Feb 22, 2025

Conversation

alexrp
Copy link
Member

@alexrp alexrp commented Feb 15, 2025

This is a correctness issue: When -fno-builtin is used, we must assume that we could be compiling the memcpy/memset implementations, so generating calls to them is problematic.

Benchmark 1 (3 runs): ./release/bin/zig build -Doptimize=ReleaseFast -Dno-lib -Denable-llvm=false -Duse-llvm=false
  measurement          mean ± σ            min … max           outliers         delta
  wall_time          12.5s  ± 69.2ms    12.4s  … 12.5s           0 ( 0%)        0%
  peak_rss           1.13GB ± 5.96MB    1.13GB … 1.14GB          0 ( 0%)        0%
  cpu_cycles         98.3G  ±  105M     98.2G  … 98.4G           0 ( 0%)        0%
  instructions        270G  ±  147M      270G  …  270G           0 ( 0%)        0%
  cache_references    227M  ±  204K      227M  …  227M           0 ( 0%)        0%
  cache_misses       82.6M  ± 1.01M     81.5M  … 83.5M           0 ( 0%)        0%
  branch_misses       344M  ± 61.8K      344M  …  344M           0 ( 0%)        0%
Benchmark 2 (3 runs): ./release-pr/bin/zig build -Doptimize=ReleaseFast -Dno-lib -Denable-llvm=false -Duse-llvm=false
  measurement          mean ± σ            min … max           outliers         delta
  wall_time          12.4s  ± 53.3ms    12.4s  … 12.5s           0 ( 0%)          -  0.2% ±  1.1%
  peak_rss           1.13GB ± 3.31MB    1.12GB … 1.13GB          0 ( 0%)          -  0.6% ±  1.0%
  cpu_cycles         98.3G  ±  165M     98.2G  … 98.5G           0 ( 0%)          +  0.0% ±  0.3%
  instructions        270G  ± 42.7M      270G  …  270G           0 ( 0%)          +  0.1% ±  0.1%
  cache_references    225M  ±  329K      225M  …  225M           0 ( 0%)          -  0.9% ±  0.3%
  cache_misses       82.4M  ±  187K     82.1M  … 82.5M           0 ( 0%)          -  0.4% ±  2.0%
  branch_misses       344M  ± 6.84M      339M  …  351M           0 ( 0%)          -  0.2% ±  3.2%

@alexrp alexrp requested review from andrewrk and jacobly0 February 18, 2025 18:44
@alexrp
Copy link
Member Author

alexrp commented Feb 18, 2025

cc @dweiller

@jacobly0
Copy link
Member

jacobly0 commented Feb 18, 2025

This is missing an if (@"inline") assert(len.toConst() != null); and checks at some of the call sites to produce a codegen error if you try to call an inline function with a variable length.

@alexrp
Copy link
Member Author

alexrp commented Feb 18, 2025

Is the length being constant actually a requirement of these intrinsics?

@jacobly0
Copy link
Member

Yes, you will get a very unhelpful llvm error if it's not.

@jacobly0
Copy link
Member

Looks like they may have removed it in llvm 19, in which case you would have to fix the definition of the intrinsics in Builder.zig, but that would lose compatibility of our emitted bitcode with older llvm versions, which I'm not sure we are ready to do.

@jacobly0
Copy link
Member

jacobly0 commented Feb 18, 2025

Even len.toConst() != null is not actually correct, it's actually

switch (builder.constant_items.items(.tag)[len.toConst().?.unwrap().constant]) {
    .positive_integer, .negative_integer => {},
    else => unreachable,
}

which could probably be a function on Constant.

@alexrp
Copy link
Member Author

alexrp commented Feb 18, 2025

Ah, I see.

Regarding backwards compatibility, in a way, we're already tied to the LLVM version we currently target anyway due to e.g. our choice of f16/f128 lowering, or the hardcoded handling of certain CPU target features for hard vs soft float. Older versions of LLVM will just straight up crash for some targets using our current lowering. (And the plan is to further change the lowering for LLVM 20 when more LLVM 19 crashes are fixed: #22003, #22013)

I guess we need to figure out how much we actually care about bitcode backwards compatibility at this stage, and if we do, maybe add a flag for the user to specify the target LLVM version.

@jacobly0
Copy link
Member

Keep in mind that Builder.zig is going to be used by more programs than just the Zig compiler, so arguments related to llvm.zig code are not relevant.

@alexrp
Copy link
Member Author

alexrp commented Feb 18, 2025

If the concern is mainly about third-party users of Builder.zig then I suppose we could just change its API to allow specifying the target LLVM version, and have the Zig compiler just specify the LLVM version we currently support. Would that be fine?

@jacobly0
Copy link
Member

Maybe, but the proper assert in Builder.zig is by far the most important requirement.

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm personally not concerned about supporting old bitcode versions (yet?). The Roc folks want to share maintenance of Builder however, so might be nice to coordinate with them about it.

There are lots of strategies for deciding how to maintain this logic, I'm open to many of them.

I'd be interested in, for starters, changing nothing about this repository, but additionally copying this logic to a separately maintained package for use by Roc and other third party projects. I don't think zig should depend on network access in order to build, since it is "dependency zero".

@nektro
Copy link
Contributor

nektro commented Feb 22, 2025

would moving it to std.zig.llvm.Builder or other internal stdlib location also be an option?

@alexrp
Copy link
Member Author

alexrp commented Feb 22, 2025

The Roc folks want to share maintenance of Builder however, so might be nice to coordinate with them about it.

Quoting @bhansconnect from the Roc Zulip:

I think we are in the same boat as you all. Bitcode backwards compatibility could be nice to have, but it is not particularly important. Changes in llvm over the years (like correct handling of i128 alignment) mean that older versions of llvm will lead to miscompilation anyway.

So yeah, progress and bug fixes are more important than trying to use old versions of llvm.

@alexrp alexrp force-pushed the llvm-nobuiltin-memcpy-inline branch from 02a64e9 to cbd7d94 Compare February 22, 2025 07:20
…-builtin.

This is a correctness issue: When -fno-builtin is used, we must assume that we
could be compiling the memcpy/memset implementations, so generating calls to
them is problematic.
@alexrp alexrp force-pushed the llvm-nobuiltin-memcpy-inline branch from cbd7d94 to 21eb757 Compare February 22, 2025 14:06
@jacobly0
Copy link
Member

jacobly0 commented Feb 22, 2025

I'd be interested in, for starters, changing nothing about this repository, but additionally copying this logic to a separately maintained package for use by Roc and other third party projects. I don't think zig should depend on network access in order to build, since it is "dependency zero".

Note that I was also thinking about the use case of no longer linking the compiler with libLLVM and supporting using the system llvm toolchain. This would require continuing to support the oldest version of llvm still in use by common distros. If the plan is to only support some newer llvm package provided by us and compiled like zig bootstrap, maybe this isn't a concern.

@andrewrk
Copy link
Member

would moving it to std.zig.llvm.Builder or other internal stdlib location also be an option?

I think that is quite a sensible option. It can be defined to be an unstable API that suits whatever needs the compiler has, and if another project wants API stability beyond locking the Zig version, or additional changes, they can maintain a fork.

For Roc's purposes, I predict they can start by directly depending on the std lib API, however, inevitably they will want to make some enhancement or fix some bug, and then will be forced to copy the implementation into their codebase. However, if they are diligent in upstreaming those enhancements and fixes, then eventually could delete the fork when a new zig version is released.

For system toolchain compatibility, the only thing that is needed is for Zig releases to continue to keep up with LLVM releases. A given system with old llvm should have old (matching) zig. I think the use case of new, upstream-provided zig mixed with old, system-provided clang is not particularly valuable. The purpose of system toolchains is primarily to build system packages. Upstream-provided zig can and should be self-sufficient.

@alexrp
Copy link
Member Author

alexrp commented Feb 22, 2025

For system toolchain compatibility, the only thing that is needed is for Zig releases to continue to keep up with LLVM releases. A given system with old llvm should have old (matching) zig. I think the use case of new, upstream-provided zig mixed with old, system-provided clang is not particularly valuable. The purpose of system toolchains is primarily to build system packages. Upstream-provided zig can and should be self-sufficient.

I think Jacob's point about system toolchains is not too far-fetched. Keep in mind that e.g. Debian/Ubuntu carry like 5-6 different LLVM versions in their repositories at any given time, and opt/llc/lld could be symlinked to any of them. Once we've removed the libLLVM dependency, it seems fairly realistic that someone could build Zig from source without thinking to also build a recent LLVM from source. But even with a system Zig and system LLVM, it's worth noting that on my Ubuntu 24.04 system, /usr/bin/llc is still LLVM 18. So while I don't think this is a huge concern until we actually drop the libLLVM dependency, once we do, we should probably seriously consider supporting the 2-3 most recent LLVM versions.

@andrewrk
Copy link
Member

andrewrk commented Feb 22, 2025

But even with a system Zig and system LLVM, it's worth noting that on my Ubuntu 24.04 system, /usr/bin/llc is still LLVM 18.

What's your system Zig in this case? Is it not the one that links LLVM 18?

Edit: ubuntu 24.04 doesn't have a zig package, so I don't know what you mean by "system zig".

@alexrp
Copy link
Member Author

alexrp commented Feb 22, 2025

I indeed don't have a system Zig. To clarify, Ubuntu 24.04 has packages for LLVM 14 through 19, but the default tools are still LLVM 18. That means that if we were in a hypothetical world where Ubuntu 24.04 had a system Zig that didn't depend on libLLVM, it would more than likely be a version that's expecting to use LLVM 19, but e.g. /usr/bin/llc is LLVM 18 (but could also be any other version if the user changed the symlink).

@andrewrk
Copy link
Member

I don't buy it. If they have multiple versions of LLVM then they can have multiple versions of zig, with a one-to-one correspondence. Or at least the version of system zig will have a corresponding LLVM on the system.

@alexrp alexrp merged commit 5e203e1 into ziglang:master Feb 22, 2025
9 checks passed
@alexrp alexrp deleted the llvm-nobuiltin-memcpy-inline branch February 22, 2025 23:51
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.

4 participants